New Check: OneTopLevelClassCheck

[maxvetrenko]

http://google-styleguide.googlecode.com/svn/trunk/javaguide.html#s3.4.1-one-top-level-class

Each top-level class resides in a source file of its own.

[romani]

1)

• Reference:<a

Should be : Official description of term top-level.

2) Message should be: Top-level class {0} has to reside in its own source file.

3) where is HTML update ? Please put in each issue on github notes:

• "Update HTML page"
• "make sure that UTs code coverage is about 90% at file:///home/rivanov/java/git-others/max-vetrenko/checkstyle/target/site/cobertura/index.html" DO NOT FORGET THAT TO DO !!!

4) Squash all commits in one and prepare for code review .

[maxvetrenko]

1. Done
2. Done
3. ??
4. Done

[maxvetrenko]

1. Updated site, code coverage is 100%

[romani]

1) I do not see any changes at xdoc files in your commit or branch.

2) put "\n" before "@author ...."

3)

if (mTopLevelClassCounter > 1) {

this mean that private top-level class will be placed before public class - you will put an error on public class - that is not right

4)

return aClassDef.getParent() == null;

what is the reason to subscribe to all classes and enums if you are interested in top-level only ? why you need to examine all inner/nested classes ? Please override method to get event on begin of the tree and examine child nodes of root. Do place violation on non-public class.

[maxvetrenko]

We can't declare top-level class as a private. If we try it, we'll have a compile-time error because top-level class must have a package level access. Top-level class can declare as a public, final or protected. So, we can call a root token, climb and recursively call findFirstToken(TokenTypes.CLASS_DEF) and findFirstToken(TokenTypes.ENUM_DEF) to find another top-level classes. And if count of top-level classes would be more than one, Check will put an warning at the second, third etc top-level classes.

Prove from Oracle "It is a compile-time error if a top level type declaration contains any one of the following access modifiers: protected, private, or static." http://docs.oracle.com/javase/specs/jls/se7/html/jls-7.html#jls-7.6

[romani]

We can't declare top-level class as a private.

Sorry my typo, you can write package(no modificator) level class as sibling to public class.

[maxvetrenko]

Updated xdoc files.

[maxvetrenko]

It's impossible to set EOF (ROOT) node as a default token: return new int[] { TokenTypes.EOF, }; I can't understand why.

[romani]

Because End Of File(EOF) is not a Java symbol.

You should use, special methods:

    /**
     * Called before the starting to process a tree. Ideal place to initialise
     * information that is to be collected whilst processing a tree.
     * @param aRootAST the root of the tree
     */
    public void beginTree(DetailAST aRootAST)
    {
    }

    /**
     * Called after finished processing a tree. Ideal place to report on
     * information collected whilst processing a tree.
     * @param aRootAST the root of the tree
     */
    public void finishTree(DetailAST aRootAST)
    {
    }

[rdiachenko]

Checks that each top-level class or enum resides in a source file of its own. TokenTypes.CLASS_DEF, TokenTypes.ENUM_DEF,

You missed interfaces

Official description of term top-level

Change to "Official description of a 'top-level' term"

An example of how to configure the check is:

Change to "An example of check's configuration:"

An example of how to configure the check to work only with classes is:

Change to "An example of check's configuration applied only to classes:"

Contains count of Top-level classes in java file

Change to "Contains a number of top-level classes in a java source file"

if (isTopLevelClass(aAST)) {

• mTopLevelClassCounter++;
• if (mTopLevelClassCounter > 1) {
• aAST.findFirstToken(TokenTypes.CLASS_DEF);
• final String className = aAST.getLastChild().
• getPreviousSibling().getText();
• log(aAST.getLineNo(), "one.top.level.class", className);
• }
• }

I don't understand this logic, let's have a skype call

Checks if class declaration is top-level type.

Change to "Checks if a class is a top-level one"

@return true, if class declared ad a top-level type.

Change to " @return true, if a specified class is a top-level one."

Your tests and test inputs don't cover all the cases

Let's discuss

[rdiachenko]

I missed this part: https://github.com/maxvetrenko/checkstyle/commit/3f91f0c6cf4522f532be611c2f0de2a57c865480

Max ALWAYS squash your commits before giving code to review. Anyway, there are notes that are still present, please fix them

[rdiachenko]

 -            <id>ant-phase-verify</id>
 -            <phase>verify</phase>
 -            <goals>
 -              <goal>run</goal>
 -            </goals>
 -            <configuration>
 -              <target>
 -                <property name="mvn.project.build.directory" value="${project.build.directory}" />
 -                <property name="mvn.project.version" value="${project.version}" />
 -                <property name="mvn.runtime_classpath" refid="maven.runtime.classpath" />
 -                <ant antfile="ant-phase-verify.xml" />
 -              </target>
 -            </configuration>
 -          </execution>```

Why did you comment it out?

private TreeMap<Integer, String> mClassCollector =

• new TreeMap<Integer, String>();

Program to interfaces

DetailAST curNode = aRootAST;

Change name to "currentNode"

private boolean mTopLevelPublicClass

Change name to "topLevelPublicTypeFound"

+            boolean skipFirstType = false; //to skip first top-level type
 +            Iterator<Map.Entry<Integer, String>> iterator = mClassCollector.entrySet().iterator();
 +            while(iterator.hasNext())
 +            {
 +                Map.Entry<Integer, String> entry = iterator.next();
 +                if (skipFirstType) {
 +                    log(entry.getKey(),
 +                            "one.top.level.class", entry.getValue());
 +                } else {
 +                  iterator.remove();
 +                  skipFirstType = true;
 +                }
 +            }

It can be written simplier:

Iterator<Map.Entry<Integer, String>> iterator = mClassCollector.entrySet().iterator();

        if (iterator.hasNext()) {
            iterator.next(); // skip first top-level type
        }

        while (iterator.hasNext()) {
            Map.Entry<Integer, String> entry = iterator.next();
            log(entry.getKey(), "one.top.level.class", entry.getValue());
        }

public void testGood() public void testBad() public void testBad_enumClass()

Bad names

[maxvetrenko]

Why did you comment it out?

I commented it to temporally skip checkstyle verify to boost build, because my PC has a low-performance. Before pull-request I'll verify my code with checkstyle.

The rest is done.

[rdiachenko]

if (currentNode.getType() == TokenTypes.CLASS_DEF

• | currentNode.getType() == TokenTypes.ENUM_DEF
• | currentNode.getType() == TokenTypes.INTERFACE_DEF)

Use short-circuit operators

Checks that each top-level class or enum resides in a source file of its own.

You forgot to mention about interfaces

mTopLevelPublicTypeFound

change to mPublicTypeFound

Check if type has public level access.

Change to "Checks if a type is public"

modifiers of type node.

Change to "Type node"

ture, if type has public level access.

Change to "true if a type has a public access level modifier"

/* True, if java file contains top-level public type/

Change to "true if a java source file contains a type with a public access level modifier"

/* Contains top level types and line numbers of there declaration/

Change to "Mapping between type names and line numbers of the type declarations"

DetailAST aModifireNode

Change to "aTypeNode"

mClassCollector

Change to "mLineNumberTypeMap"

[rdiachenko]

• Checks that each top-level class or enum resides in a source file of its own.

You forgot about interfaces

[romani]

1) Why formatting was missed ? No empty lines between fields declarations No empty line between methods declarations.

2)

if (isPublic(currentNode.
                        findFirstToken(TokenTypes.MODIFIERS)))

"is Public ?" question could applied to AST only, if want to check public in list of modifiers question should be "contains Public ?"

    private boolean isPublic(DetailAST aTypeNode)
    {
        return aTypeNode.findFirstToken(TokenTypes.LITERAL_PUBLIC) != null;
    }

isPublic == findFirst ????

3)

// skip first top-level type

Describe this nuance in JavaDoc do not expect that all developers know all nuances(corner cases) of Java.

4) to avoid copy-paste in finishTree I would recommend to remove one element from base on mPublicTypeFound.

[maxvetrenko]

1. Done
2. Done.
3. Done
4. Done

[romani]

Read this again and DO IT.

3) // skip first top-level type Describe this nuance in JavaDoc do not expect that all developers know all nuances(corner cases) of Java.

[maxvetrenko]

3) I described this case in the Javadoc. Commit

[romani]

ok for PR, please do not forget to create issue in Checkstyle repo and make a link to it in commit message.

[maxvetrenko]

Merged