Update for CustomDeclarationOrderCheck. Issue 73.

[Измаилов Баратали]

1) group Fields with assigning to Anonymous classes;

private successListener = new Listener {
...
// some anonymous class definition, could be few methods definition
....
}
private int value;
private successListener2 = new new Listener {
...
// some anonymous class definition, could be few methods definition
....
}

Name for macros is Field_ann_class

2) Setter/Getter (base on bean notation). Notation detection was taken from https://github.com/sevntu-checkstyle/sevntu.checkstyle/blob/master/sevntu-checks/src/main/java/com/github/sevntu/checkstyle/checks/coding/SimpleAccessorNameNotationCheck.java
Marcos: Getter_setter
expected order of declarations - all get/set are grouped and not mixed with other methods of class.

public int setX(){}
public int getX(){}
public int setY(){}
public int getY(){}
 

3) Coverage of nested interfaces, nested enumerations
Macros: InnerInterfaces, InnerEnums.

public class MyClass {

    interface CrazyInterface{ .... }
        ....
    }

    enum HiddenEnum{ .... }
        ....
    }
}

4) Coverage nested classes in methods
Option that are "true" by default: ignoreInnerClassesInMethods

Example of bad code:

private methodX (...) {
    class NameC{ ... }
    ...
}

Link to GitHub

[Roman Ivanov]

Do you See consistency in naming ?

>Marcos: Getter_setter

>Macros: InnerInterfaces, InnerEnums.

>Name for macros is Field_ann_class

and please do the last as DeclareAnnonClassField

Please fix.

--
You received this message because you are subscribed to the Google Groups "Sevntu Checkstyle" group.
To unsubscribe from this group and stop receiving emails from it, send an email to sevntu-checkst...@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.

--
Hello ,

Thanks,

Roman Ivanov.

[Измаилов Баратали]

I've changed names to:

• GetterSetter
• InnerInterface
• InnerEnums
• DeclareAnnonClassField

https://github.com/baratali/sevntu.checkstyle/commit/40720b36a73b1257a8fe6d864ed064a77d8c3fd8

2013/10/28 Roman Ivanov <romani...@gmail.com>

[RomanIvanov]

sorry for delay, I need more time to understand you code ...

[RomanIvanov]

https://github.com/baratali/sevntu.checkstyle/blob/40720b36a73b1257a8fe6d864ed064a77d8c3fd8/sevntu-checks/src/main/java/com/github/sevntu/checkstyle/checks/coding/CustomDeclarationOrderCheck.java

1.  <li>"GetterSetter" to denote the group of getter and setter methods</li>

Please extend doc to explain how getter/setter is detected 

 

2. Field(public.*final.*)

We need get user know that such approach is rely of strict order of modifiers - so ModifiersOrderCheck is recommended to use

3. public int compare(DetailAST o1, DetailAST o2)

Can we just do return o1.getLineNo() - o2.getLineNo() ?

4.  private static final int INITIAL_STATE = 0;

Please apply your check to its own code !  There is a complete mess.

What for define constant is it is used only ones ?

5. 

/**
     * Stack of lists to keep getter list for certain class definition.
     */
    private Deque<List<DetailAST>> mGetters = new LinkedList<List<DetailAST>>();

Stack ?

6. private String mFieldPrefix = "";

Where is JavaDoc  ?

7.

        if (!mCustomOrderDeclaration.isEmpty()) {
            mCustomOrderDeclaration.clear();
        }

Do we really need this check for empty ?

8. public void setFieldPrefix(String aFieldPrefix) {

JavaDoc ?

9.         String classMember;

        String regExp;

Why Pascal approach is used? 

10.

        switch(aInputMemberName) {
        case "field":

Switch by string - Java7. Please downgrade to java5. We still not decided with update https://groups.google.com/forum/?hl=en#!topic/checkstyle-devel/nd8rWCtJ7Tg

 

11.

 public void setIgnoreRegExCase(final boolean aCaseInsensitive)
    {
        if (aCaseInsensitive) {

If I set False - mean do nothing ? Is it expected?

12.

final HashSet<String> classMembers = new HashSet<String>();

final Set<String> class...........

13. //add Tokens

no trailing comments

14.

else {
                defaultTokens[index] = defaultTokens[0];
            }

Suspicious logic, lets discuss.

15.

                    mObjectBlockTokens.push(objBlock);
                    mSetters.push(new ArrayList<DetailAST>());
                    mGetters.push(new ArrayList<DetailAST>());

What about to have one collection of mClass, where we have inner object with list of setters and getters.

16.

                        && !(mIgnoreInnerClassesInMethods
                        && isClassDefInMethodDef(aAST)))

Copy-paste

17. checkOrderLogic(aAST);

Strange name , looks like logging will be there, but that not reflected to name.

In my mind you have to return flag from that function and do logging on that level.

18.

            if (mClassStates.isEmpty()) {
                mClassRoot = true;
            }

Lets discuss ... I do not understand that 

19. * Check class declaration order with custom declaration order.

Please rewrite 

20. final int currentState = getPosition(aAST);

So it is state or position?  looks like it is position.

21.

    /**
     * Examine method definition. If it is getter or setter,
     * then adds this method to collection.
     * @param aMethodDefAst DetailAST of method definition.
     */
    private void findGetterSetter(DetailAST aMethodDefAst) {

Reconcile  name and comment ... examine vs find.

22. "GetterSetter"

Copy-paste

23.

if (nameOfGettingField != null
                            && verifyFieldAndMethodName(nameOfGettingField,
                                    methodName))

Not all methods that start from "get" are getters - please review getter detection in appropriate Check.

24.

    /**
     * Use for recursive tree traversal from first child of current tree top.
     *
     * @param aAST current DetailAST state, first child of current tree top.
     * @return the unit modifiers and annotation list.
     */
    private String concatLogic(final DetailAST aAST)

Reconcile name with Doc .... concat vs traverse ...

25.   private static class FormatMatcher

What the lesson you took from writing that Check ? ... mess in order

[Измаилов Баратали]

https://github.com/baratali/sevntu.checkstyle/commit/72811381dc74d2744b27d8cadf2690f5ee0ee46f
1. Done.
2. Done.
3. Done.
4. Redundant constant. Deleted.
5. Yes. Put object when we enter to class node and pop object when we leave class node.
6. Done.
7. Redundant condition. Deleted.
8. Done.
9. Edited.
10. Done.
11. Yes. False is default value. We shouldn't initialize pattern with CaseSensitive value.
12. Done.
13. Done.
14. Edited.
15. Added private class(GetterSetterContainer) with two collections(getters and setters) and proper methods.
16. Edited.
17. Renamed.
18. Edited.
19. Done.
20. It is position. Renamed.
21. Edited.
22. All macros were moved to constants.
23. I know. I took getters/setters detection from SimpleAccessorNameNotationCheck.
24. Renamed.
25. Done.

New macro MainMethod was added.



среда, 6 ноября 2013 г., 8:08:54 UTC+2 пользователь RomanIvanov написал:

[Измаилов Баратали]

from Roman:

1)
 * It is important to write exact order of modifiers in rules. So rule
 * <code><i>Field(public final)</i></code> does not match to <code><i>final public value;</i></code>.
 * <a href='http://checkstyle.sourceforge.net/config_modifier.html'>ModifierOrderCheck</a>
 * is recommended to use.

Provide more specified ref.

2)
private final ArrayList<FormatMatcher> mCustomOrderDeclaration =
        new ArrayList<FormatMatcher>();

should be List<FormatMatcher>.

3)
private final FastStack<Integer> mClassPositions =
        new FastStack<Integer>();

change implementation of stack because FastStack uses synchronized methods.

4)
isNotIgnoredClass() -> isIgnoredClass()

5)
checkPositionAndLog()

split this method into two methods.

6)
Class position, getters, setters may be combined in one entity(for example, private class ClassDetail) with validate() method.

7)
        final int[] classMembers = new int[size + 1];

Bad name. It contains token types.

8)
private int getPosition(final DetailAST aAST)

Unclear name. Extend method name.

9)
            if (statementsAst != null
                    && containsOnlyReturn(statementsAst))

Getter have to return class field, but it can contain another statements except return statement.

10)
            if (statementsAst != null
                    && containsOnlyExpression(statementsAst))

Setter have to set class field, but it can contain another statements.

11)
getUniteModifiers() -> getCombinedModifiers()

12)
getNameOfSettingField() -> getNameOfSetterField()
getNameOfGettingField() -> getNameOfGetterField()

13)
            if (leftPart.getType() == TokenTypes.IDENT) {
                ...
            }
            else {
                if (leftPart.getType() == TokenTypes.DOT) {
                    ...
                }
            }

to

            if (leftPart.getType() == TokenTypes.IDENT) {
                ...
            }
            else if (leftPart.getType() == TokenTypes.DOT) {
                ...
            }

[Измаилов Баратали]

Link to commit: https://github.com/baratali/sevntu.checkstyle/commit/a6a4169a55231997cbec95d9e4f219aa4fe321d5

1) Done.
2) Done.
3) This stack was deleted.
4) Done.
5) Done.
6) Created ClassDetail class that contains class position (so we can delete mClassPositions collection), list of getters, list of setters.
7) Renamed to tokenTypes.
8) Renamed to getPositionInOrderDeclaration(...).
9) Changed detection of getters.
10) Changed detection of setters.
11) Done.
12) Done.
13) Done.

понедельник, 20 января 2014 г., 1:06:34 UTC+2 пользователь Измаилов Баратали написал:

[RomanIvanov]

1) 

* public void setValue(int newValue) {

What about complicated setter s and getters ?

   public void setValue(int newValue) {
validate()
value = newValue;
}

public void setObj(Object obj) {
if (!initialized) {
throw Exception();
}
this.obj = obj;
}

2) private boolean mCheeckInnerClasses;

typo

3) 

private boolean mIgnoreInnerClassesInMethods = true;

I lost a reason of this option to be FALSE, do not remember this , lets discuss.

4) 

public void setIgnoreRegExCas

lets discuss, I do not like this to be setter for mCompileFlags, or I do not like name of it.

Why set of it was skip in first IF in that method?

5) 

Parsing input line with custom declaration order into massive.

 What is massive ? 

6) 

mClassDetails.push(new ClassDetail());

Very strange code. We need to postpone pushing empty object to the point where it is going to be used, or lets discuss

7) 

if (mCheckGettersSetters) {
   collectGetterSetter(aAST);
}
logWrongOrderedElement(aAST);

Log warnings without condition ?

it have to be 

if (isWrong(...))

{

 logWrong(....)

}

8) 

* Returns true when getter is correct.

Doc have to explain what is "correct" getter, your decision is subjective I pretty sure. The same for setter.

[Измаилов Баратали]

GitHub commit: https://github.com/baratali/sevntu.checkstyle/commit/079349cadd707bf2ea4357b2053f9463f7496cc6
1) JavaDoc was extended.
2) Renamed.
3) Field was deleted.
4) This method was rewrote and renamed to setCaseSensitive(boolean)
5) Edited.
6) I have no idea where to move it. mClassDetails is used in visitToken() and initialized in visitToken().
7) Done.
8) Done.

вторник, 4 февраля 2014 г., 19:24:45 UTC+2 пользователь RomanIvanov написал:

[RomanIvanov]

please call me in Skype to discuss "6)"

[RomanIvanov]

forget about "6)", now it is clear for me a reason.

I will do testing of binaries soon.

[RomanIvanov]

from Roman:

1) I failed to use " ### DeclareAnnonClassField(.*) " for DMT project.

No matter there I put it in patter, no violations found - very strange - I do not believe in this.

2) AbstractCommand.java in DMT project

@Override  //WARNING IS HERE , BUT THIS IS NOT GETTER

public ISession getSession() {

.....

            }

rule:

MainMethod(.*) ### Field(private static final long serialVersionUID) ### Field((private|protected) final Log ([\w]*L|l)og|private static final Log [\w]*LOG) ### Field((private|protected) final Logger ([\w]*L|l)ogger|private static final Logger [\w]*LOGGER) ### Field((private|protected) final Logger ([\w]*L|l)og|private static final Logger [\w]*LOG) ### Field(public static final) ### Field((private|protected) static final) ### Field(@Autowired.* public) ### Field(@Inject.* public) ### Field(public.*) ### Field(public) ### Field(private final) ### Field(private.*) ### Field(private) ### Field(.*) ### Method((public|protected)?(\w*\s)?abstract\s) ### Method(public static .*(new|edit|create|open|clone).*) ### Ctor(public) ### Ctor(private) ### Method(@Autowired.* public) ### Method(@Inject.* public)  ### GetterSetter(.*) ### Method(.*)

3) Discuss with team idea to put all Enums even public to the bottom of class 

.... ### InnerEnum(.*) ### InnerInterface(.*) ### InnerClass(.*)

FYI: I plan to use following template for all our classes

MainMethod(.*) ### Field(private static final long serialVersionUID) ### Field((private|protected) final Log ([\w]*L|l)og|private static final Log [\w]*LOG) ### Field((private|protected) final Logger ([\w]*L|l)ogger|private static final Logger [\w]*LOGGER) ### Field((private|protected) final Logger ([\w]*L|l)og|private static final Logger [\w]*LOG) ### Field(public static final) ### Field((private|protected) static final) ### Field(@Autowired.* public) ### Field(@Inject.* public) ### Field(public.*) ### Field(public) ### Field(private final) ### Field(private.*) ### Field(private) ### Field(.*) 
### DeclareAnnonClassField(.*) 
### Method((public|protected)?(\w*\s)?abstract\s) ### Method(public static .*(new|edit|create|open|clone).*) ### Ctor(public) ### Ctor(private) ### Method(@Autowired.* public) ### Method(@Inject.* public) 
### GetterSetter(.*)
### Method(.*) 
### InnerEnum(.*) ### InnerInterface(.*) ### InnerClass(.*)

from Bratali:

https://github.com/baratali/sevntu.checkstyle/commit/7d075e892bc734c4cc3cca7fb015a80d004d7570

1) Fixed. Problem was that anonymous class field also matched Field(...) pattern.
2) Fixed. Problem was that local variable hides class field.
3) Somebody think that it would be convenient to define inner enums/interfaces at the beginning of class.

[RomanIvanov]

Works fine !!! Please be welcome with PR.

Latest format:

MainMethod(.*) ### Field(private static final long serialVersionUID) ### Field((private|protected) final Log ([\w]*L|l)og|private static final Log [\w]*LOG) ### Field((private|protected) final Logger ([\w]*L|l)ogger|private static final Logger [\w]*LOGGER) ### Field((private|protected) final Logger ([\w]*L|l)og|private static final Logger [\w]*LOG)  ### Field(public static final) ### InnerEnum(public .*) ### Field((private|protected) static final) ### Field(@Autowired.* public) ### Field(@Inject.* public) ### Field(public.*) ### Field(public) ### Field(private final) ### Field(private.*) ### Field(private) ### Field(.*) ### DeclareAnnonClassField(.*) ### Method((public|protected)?(\w*\s)?abstract\s) ### Method(public static .*(new|edit|create|open|clone).*) ### Ctor(public) ### Ctor(private) ### Method(@Autowired.* public) ### Method(@Inject.* public) ### GetterSetter(.*) ### Method(.*)  ### InnerEnum(private .*) ### InnerInterface(.*) ### InnerClass(.*)

in multy-line format:

MainMethod(.*) 

### Field(private static final long serialVersionUID) ### Field((private|protected) final Log ([\w]*L|l)og|private static final Log [\w]*LOG) ### Field((private|protected) final Logger ([\w]*L|l)ogger|private static final Logger [\w]*LOGGER) ### Field((private|protected) final Logger ([\w]*L|l)og|private static final Logger [\w]*LOG)  

### Field(public static final) 

### InnerEnum(public .*) 

### Field((private|protected) static final) 

### Field(@Autowired.* public) 

### Field(@Inject.* public) 

### Field(public.*) 

### Field(public) 

### Field(private final) 

### Field(private.*) 

### Field(private) 

### Field(.*) 

### DeclareAnnonClassField(.*) 

### Method((public|protected)?(\w*\s)?abstract\s) 

### Method(public static .*(new|edit|create|open|clone).*) 

### Ctor(public) 

### Ctor(private) 

### Method(@Autowired.* public) 

### Method(@Inject.* public) 

### GetterSetter(.*) 

### Method(.*)  

### InnerEnum(private .*) 

### InnerInterface(.*) 

### InnerClass(.*)

[Измаилов Баратали]

Pull Request: https://github.com/sevntu-checkstyle/sevntu.checkstyle/pull/174

--

[RomanIvanov]

GetterSetter is a bit buggy need to be updated - issue.

Final approved format by team is:

MainMethod(.*) ###

Field(private static final long serialVersionUID) ###

Field((private|protected) final Log ([\w]*L|l)og|private static final Log [\w]*LOG) ###

Field((private|protected) final Logger ([\w]*L|l)ogger|private static final Logger [\w]*LOGGER) ###

Field((private|protected) final Logger ([\w]*L|l)og|private static final Logger [\w]*LOG) ###

InnerEnum(public .*) ###

Field(public static final) ###

DeclareAnnonClassField(public static final) ###

Field((private|protected) static final) ###

DeclareAnnonClassField((private|protected) static final) ###

Field(@Autowired.* public) ###

Field(@Inject.* public) ###

Field(public.*) ###

Field(public) ###

Field(private final) ###

Field(private.*) ###

Field(private) ###

Field(.*) ###

Method((public|protected)?(\w*\s)?abstract\s) ###

Method(public static .*(new|edit|create|open|clone).*) ###

Ctor(public) ###

Ctor(private) ###

Method(@Autowired.* public) ###

Method(@Inject.* public) ###

[Алексей Нестеренко]

Updated according to the issue.

Treating setters now covers even narrow cases, which were represented in the current issue.

Changes: https://github.com/alexkravin/sevntu.checkstyle/compare/CustomDeclarationOrder_%23190

вторник, 24 июня 2014 г., 2:37:16 UTC+4 пользователь RomanIvanov написал:

[Алексей Нестеренко]

пятница, 27 июня 2014 г., 18:39:57 UTC+4 пользователь Алексей Нестеренко написал:

[Roman Ivanov]

Hi Alexey,

https://github.com/alexkravin/sevntu.checkstyle/compare/CustomDeclarationOrder_%23190

this diff contains reference to NestedTernary - please isolate diff on Custom Declaration order.

thanks,

Roman Ivanov

--

You received this message because you are subscribed to the Google Groups "Sevntu Checkstyle" group.
To unsubscribe from this group and stop receiving emails from it, send an email to sevntu-checkst...@googlegroups.com.

For more options, visit https://groups.google.com/d/optout.

[Алексей Нестеренко]

Done.

https://github.com/alexkravin/sevntu.checkstyle/compare/CustomDeclarationOrder_%23190

пятница, 27 июня 2014 г., 19:45:02 UTC+4 пользователь RomanIvanov написал:

[Алексей Нестеренко]

вторник, 1 июля 2014 г., 13:47:30 UTC+4 пользователь Алексей Нестеренко написал:

[Даниил Ярославцев]

Build works well, but:

1. Please, add JavaDoc to new 'getFieldName' method.
2. Please rewrite method:

 private static String getNameOfSetterField(DetailAST aAssignAst)

It now could get methodCallAst, and not only assignAst, so, this method should be splitted to 2 parts to avoid mixing different logic branches:
one part for processing assign node, another - for processing method calls. 

[Алексей Нестеренко]

Updated.

https://github.com/alexkravin/sevntu.checkstyle/compare/CustomDeclarationOrder_%23190

вторник, 1 июля 2014 г., 14:49:41 UTC+4 пользователь Даниил Ярославцев написал:

[Даниил Ярославцев]

1. if (leftPart.getChildCount() == 2) {

                if ("this".equals(leftPart.getFirstChild().getText())) {

                    nameOfSettingField = leftPart.getLastChild().getText();

                }

            }

-->

if (leftPart.getChildCount() == 2 && "this".equals(leftPart.getFirstChild().getText())) {

      nameOfSettingField = leftPart.getLastChild().getText();

}

same for code:

if (leftPart.getChildCount() == 2) {

            if ("super".equals(leftPart.getFirstChild().getText())) {

                nameOfSettingField = getFieldName(leftPart);

            }

        }

2. Lines 947, 968:

final DetailAST leftPart = assignAst.getFirstChild();

It is always a DOT ast, please name it appropriately

3. Please fix method JavaDoc:

    /**
     * Verify that exists assignment to field among statements.
     * @param aStatementsAst DetailAST of statements (SLIST).
     * @param aFieldName name of target field.
     * @return true if there is assignment to aFieldName in aStatementsAst.
     */
    private static boolean isFieldUpdate(DetailAST aStatementsAst, String aFieldName)

[Алексей Нестеренко]

1. Done.

if (methodCallDot.getChildCount() == 2 

                    && "this".equals(methodCallDot.getFirstChild().getText())) { 

                    nameOfSettingField = methodCallDot.getLastChild().getText();

            }

2. Done.

final DetailAST methodCallDot = assignAst.getFirstChild();

3. Done.

/**

     * Verify that exists updating of a field.

     * @param aStatementsAst DetailAST of statements (SLIST).

     * @param aFieldName name of target field.

     * @return true if there is updating of aFieldName in aStatementsAst.

     */

    private static boolean isFieldUpdate(DetailAST aStatementsAst, String aFieldName)

вторник, 1 июля 2014 г., 15:31:20 UTC+4 пользователь Даниил Ярославцев написал:

[Даниил Ярославцев]

I have no objection to latest commit: https://github.com/alexkravin/sevntu.checkstyle/commit/49e5a942db2280d185679ed39b42bcff5f2f2943
Roman, do you have any?

[Roman Ivanov]

Hi Alexey,

The code looks ok, two small items :

1) 

I know that is not your changes , but please do this as you are there

            DetailAST nextStatement = currentStatement.getFirstChild();

            while ((currentStatement != null) && (nextStatement == null)) {
                nextStatement = currentStatement.getNextSibling();
                if (nextStatement == null) {
                    currentStatement = currentStatement.getParent();
                }
            }

that should be a separate method, as it does not have any relation to validation logic.

2) Please update Description for Eclipse plugin for this Check, I remember it is missed.

And the most complicated Check does not have appropriate description :( - sad fact.

https://github.com/alexkravin/sevntu.checkstyle/blob/CustomDeclarationOrder_%23190/eclipsecs-sevntu-plugin/src/com/github/sevntu/checkstyle/checks/coding/checkstyle-metadata.properties#L22 

thanks,

Roman Ivanov

​

[Алексей Нестеренко]

Hi Roman,

1. Moving this part of code to the separate method is bigger problem than it appears, because in that case there would be changing value of method param in caller method, The code will be conversely more complicated, so I suggest to leave this while statement and assignment in it on it's place if you have more specific objections, please comment.

2. Updated checkstyle-metadata.properties.

https://github.com/alexkravin/sevntu.checkstyle/compare/CustomDeclarationOrder_%23190#diff-0

среда, 2 июля 2014 г., 9:24:06 UTC+4 пользователь RomanIvanov написал:

[Roman Ivanov]

Hi Alexey,

ok, please do PR.

thanks,

Roman Ivanov
​

[Даниил Ярославцев]

Alexey, good work! PR https://github.com/sevntu-checkstyle/sevntu.checkstyle/pull/193 was applied to main repo, thread closed.
I have added you to project contributors: https://github.com/sevntu-checkstyle/sevntu.checkstyle