issue #339

[Vladislav Lis]

according to issue #339, new Check: StaticMethodCandidate
please, review UT and input

thanks, Vladislav

[Roman Ivanov]

1) As it is input , please presume that you already have a Check and provide comment is Input as "// violation", " // ok, as ...... ", and place that as trailing line comment for line where you expect that violation

2) "not checked" - I do not not understand that meaning, see point "1)"

3) 

> +class ClassA {

+    int instanceVariable;

+    static int classVariable;

please name it as instanceAField, classAField, ...... the same for other fields to make it absolutely clear where they are declared.

4) 

Please try to write there algorithm by words and share at this thread.

​

thanks,

Roman Ivanov

[Vladislav Lis]

1) 2) 3) Done
4)
1. Collect information about static variables and methods for every class
2. Manually traverse METHOD_DEFs
    2.1 Stop if method doesn't have modifier 'private'
    2.1 If VARIABLE_DEF is met, add it to the list of acceptable variables for the current method
    2.2 Stop if one of the following is met:
        1) LITERAL_THIS
        2) METHOD_CALL which doesn't have DOT as its first child and is not in the list of methods, collected in p.1 (including lists for all parent classes in case of nested class)
        3) usage of a variable, which is not found in both lists, made in p.1 and p.2.1 (including lists for all parent classes in case of nested class)
    2.3 If traversing ends without meeting any situation from p.2.2, then method should be static

[Roman Ivanov]

As you write "done" please provide link to that code.

Please add in your inputs and algorithm cases for static imports

Why we so validating only private methods ?

--
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.

[Vladislav Lis]

> Please add in your inputs and algorithm cases for static imports

Done: latest commit
Corrected algorithm:
1. Collect information about static imports.
2. Collect information about static variables and methods for every class.
3. Manually traverse METHOD_DEFs:
    3.1 Stop if method doesn't have modifier 'private'
    3.2 If VARIABLE_DEF is met, add it to the list of acceptable variables for the current method
    3.3 Stop if one of the following is met:
        1) LITERAL_THIS
        2) METHOD_CALL which doesn't have DOT as its first child and is not in the list of methods, collected in p.1 and p.2 (including lists for all parent classes in case of a nested class)
        3) usage of a variable, which is not found in both lists, made in p.2 and p.3.2 (including lists for all parent classes in case of a nested class)
    3.4 If traversing ends without meeting any situation from p.3.3, then method should be static

> Why we so validating only private methods ?

Other methods can be already overridden or designed to be overridden later.

thanks, Vladislav

[Roman Ivanov]

please make a test case with static import of variable (not a method)  and proceed with implementation.

think about static classes .....

[Vladislav Lis]

Please, review:
Check
Test
Input

[Vladislav Lis]

I have a problem running checkstyle-tester the new check, it throws NoClassDefFound on ScopeUtils, no matter if I use a deprecated class (com.puppycrawl.tools.checkstyle.api.ScopeUtils) or not (com.puppycrawl.tools.checkstyle.ScopeUtils). Please, see gist. Do you have any ideas why does it happen?

[Roman Ivanov]

Hi Vlad,

https://github.com/Vladlis/sevntu.checkstyle/blob/536a50459a23c8371d64fed05e8c6d34d01b787b/sevntu-checks/src/main/java/com/github/sevntu/checkstyle/checks/design/StaticMethodCandidateCheck.java

1)

if (astType != TokenTypes.VARIABLE_DEF

&& astType != TokenTypes.PARAMETER_DEF

&& astType != TokenTypes.LITERAL_NEW

&& astType != TokenTypes.EXPR

&& astType != TokenTypes.LITERAL_THIS

|| astType == TokenTypes.LITERAL_NEW

&& ast.branchContains(TokenTypes.LCURLY)) {

If you do not use braces - user need to see priority of execution by indentation.

That condition could be rewritten with two methods that will make it clear what is checked there.

2)

> // non-private or static methods are not checked and can't have static methods

please fix a comment , it is not clear why static method cant have static methods.

3)

>

private void processLiteralNew(DetailAST ast) {

if (ast.branchContains(TokenTypes.LCURLY)) {

Move condition out of method to upper level, it should show you that such condition you already have at other place.

4)

> 

// ENUM_CONSTANT_DEF cannot have static methods

frame.isShouldBeProcessed = false;

Can we skip Frame creation if we do not need it during validation ? (I did not read code thoroughly)

5)

> 

currentFrame = child;

if (!child.isClassOrEnum) { It is not clear why we use currentFrame during validatin. please explain. 6) >  if (child.hasLiteralThis) { isStaticCandidate = false; }

Can we avoid this filed usage ? can we return it as result.

---------------------------------

I will continue Code review after testing result verification.

thanks,

Roman Ivanov

​

[Roman Ivanov]

Hi Vlad,

>  it throws NoClassDefFound on ScopeUtils, no matter if I use a deprecated class (com.puppycrawl.tools.checkstyle.api.ScopeUtils) or not (com.puppycrawl.tools.checkstyle.ScopeUtils). Please, see gist. Do you have any ideas why does it happen?

Class was moved from one place to another

https://github.com/checkstyle/checkstyle/commits/master/src/main/java/com/puppycrawl/tools/checkstyle/utils/ScopeUtils.java

https://github.com/checkstyle/checkstyle/commit/ed595de84fba2db507adaf01d04cb089a5e0bac5

it was released at checkstyle-6.10

Sevntu checks use old Chekcstyle library - 6.7  , I updated it to 6.9 to match version on Eclispe-cs version. Usage of 6.11 version will result in compilation problems.

What you can do:

- update to sevntu.checkstyle code to have 6.11 version, fix all compilation errors (simple changes in package names and, some functions are moved as is to another Util class - also ease). Send me PR with changes to version bump.

- you can downgrade checkstyle-tester/pom.xml project to 6.9 version in stead of 6.12-SNAPSHOT.

thanks,

Roman Ivanov

​

[Roman Ivanov]

>  I updated it to 6.9 to match version on Eclispe-cs version.

I did revert as it fail a build, unexpectedly.

one more complication

https://travis-ci.org/sevntu-checkstyle/sevntu.checkstyle/jobs/82769137#L638 

with version bump :(.

​

[Vladislav Lis]

Hi Roman,

Commit

1) Rewritten with two methods.

2) Extended.

3) Fixed, I guess ?

4) Then we also need to skip method leaveToken() until the ast, where we stopped creating tree is left, but I've failed to determine when this particular ast is left.

The only possible way I see is to store the copy of the ast on which we have stopped tree creation and then compare it to all the left ast's to determine the moment when we need to proceed creation. I could try, if it's OK.

5) It is used in findStaticMethod(), findField(), findFrameByName(). I thought it would be better to use an instance field, which we do not use after the Frame tree is created, instead of passing an additional parameter through the recursion.

6) Done.

--

You received this message because you are subscribed to a topic in the Google Groups "Sevntu Checkstyle" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/sevntu-checkstyle/cQG-PA0WgvA/unsubscribe.
To unsubscribe from this group and all its topics, send an email to sevntu-checkst...@googlegroups.com.

[Vladislav Lis]

Also please see tester results over guava, checkstyle, sevntu-checkstyle and orekit on https://vladlis.github.io/reports/static_check/

I've confirmed some of them selectively.

[Roman Ivanov]

Hi Valislav,

I will continue review after we finish testing of code, please share html reports.

Thanks,
Roman Ivanov

[Roman Ivanov]

Hi Vladislav,

Selective confirmation is not ok.

Please remove testinputs from testing.
Run report again on one project.
Fix all suggestions, make sure it is compilable.
run report one more time, ensure that contains no violation.

Share with me:
Code before fixes
Report before your fixes
Commit that put static and resolve all violations
Report after fixes, it should have no violations.

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.

[Vladislav Lis]

Hi Roman.

I've run the tester on Orekit:
Code before fixes
Report before fixes

Commit that put static and resolve all violations

Report after fixes - it contains violations, because after the first fix additional violations appeared, so I've made another fix

Commit
Report

and one more:

Commit
Report

Checkstyle has only one violation and sevntu-checkstyle has none.

[Roman Ivanov]

Hi Vladislav,

1)

> Checkstyle has only one violation

can you share this violation ? It is interesting why Eclipse and IntelijIdea inspections missed this.

2) Please generate reports against Guava

3) Please recheck that on all projects in chekcstyle-tester you do not have Exceptions (make a violations with WARN severity and just check for ERROR count, see test-no-exceptions.sh). I do not need report - just confirm.

thanks,

Roman Ivanov

[Roman Ivanov]

Hi Vladislav,

1) Introduction of new Check in sevntu is a bit more complicated, Please update your commit to have changes in all files as at

https://github.com/sevntu-checkstyle/sevntu.checkstyle/commit/ff4fdd2c3c8002dfec38f1ad56e79756cbcb056d

2) 

private void processLiteralNew(DetailAST ast) {

if (isAnonymousClass(ast)) {

If inside a method there is IF that wrap whole body - it is 80% smth wrong.

you can move "If ()" to upper level (to CASE) ,and rename your method to be more exact.

Whole logic become more exact. I see that you try to make SWITCH looks the same for all cases, but better name method is more valuable.

3) 

public int[] getAcceptableTokens() {

return getDefaultTokens();

}

AccpetableTokens collection is always bigger then Default. Swap calls.

4)

> checkFrame(currentFrame);

..

> 

else {

currentFrame = child;

isStaticCandidate = checkFrameExpressions(isStaticCandidate, child);

} passing field as argument is used when you want to single-out method from relationship to class members, but you continue to use it as field inside. Conflict of intentions. Moreover, it is clear why you need this field during filling structure with data, but it is not clear why it is used in  private boolean findStaticMethod(DetailAST methodCall, String checkedMethodName) { Frame frame = currentFrame;

and similar methods as you copy value to local variable at the beginning, look like you already want it to be argument of methods. 

thanks,

Roman Ivanov
​

[Vladislav Lis]

Hi Roman,

1)
> can you share this violation
https://github.com/checkstyle/checkstyle/blob/master/src/main/java/com/puppycrawl/tools/checkstyle/gui/ListToTreeSelectionModelWrapper.java#L93

2) http://vladlis.github.io/reports/static_check/guava/

3) Confirmed.

Thanks,
Vlad

[Roman Ivanov]

Hi Vladislav,

> http://vladlis.github.io/reports/static_check/guava/

please show that resolving all static violations do not cause compilations problems.

thansk,

Roman Ivanov

​

[Vladislav Lis]

Hi Roman,

1) Done.

2) Moved IF the upper level.

3) Done.

4) Fixed.

5)

> please show that resolving all static violations do not cause compilations problems.

Report before the changes
Commit with the changes
Report after the changes

The latest commit of the check

Thanks,

Vlad

--

[Roman Ivanov]

While I doing codereview - please do PR to Guava :).
​

[Roman Ivanov]

Hi Vadislav,

https://github.com/Vladlis/sevntu.checkstyle/blob/1bf44df20193c4a0f4a7a9ea95e8fe3167198f7a/sevntu-checks/src/main/java/com/github/sevntu/checkstyle/checks/design/StaticMethodCandidateCheck.java

1) 

> Checks whether {@code private} methods should be declared as {@code static}.

Checks whether {@code private} methods can be declared as {@code static}.

2)

Add getRequiredTokens method

3)

> 

public void finishTree(DetailAST ast) {

checkFrame(currentFrame);

} you ignore result of method, that should be explain by comment 4) >  private boolean checkFrame(Frame frame) { boolean isStaticCandidate = true; for (Frame child: frame.children) { if (child.isShouldBeProcessed) { if (!child.children.isEmpty()) {

4.1) "child.children" sounds weird, please rename argument to "parentFrame" and name iterator as "frame"

4.2) "checkFrame" vs "isShouldBeProcessed" . should be isShouldBeChecked 

4.3) "if (!child.children.isEmpty()) {" remove this condition and let interpretator come into the method. Code will become clear, no significant performance hit.

5)

>

&& checkFrameExpressions(isStaticCandidate, child)

&& checkTypes(isStaticCandidate, child); should be  && isStaticCandidat && echeckFrameExpressions(child) && checkTypes(child); 6) > if (child.isShouldBeProcessed) { ...  if (child.isPrivateMethod) {

Why we so differentiate , I hope this is the same.

7)

> 

result = typeFrame == null || typeFrame.isShouldBeProcessed;

result &= !findTypeVariable(type, frame); combine in one statement. 8) > private static boolean findTypeVariable(String type, Frame frame) { Please use order of arguments like: "where/target/big-object", "what/item/small-object". So your method should be "findTypeVariable(Frame frame, String type)". I did not found explanation of this principle in internet, but here is example - http://docs.guava-libraries.googlecode.com/git-history/release/javadoc/com/google/common/collect/Maps.html#uniqueIndex%28java.lang.Iterable,%20com.google.common.base.Function%29 Please update all other methods, a lot of them have this problem.

9)

> 

return hasStaticMethod

&& !hasNonStaticMethod;

Please explain why we need "hasStaticMethod", link to example will be good.

10)

> 

if (parentType == TokenTypes.CLASS_DEF

|| parentType == TokenTypes.ENUM_DEF) {

result = false;

make it like "return (EXPRESSION)".

11)

> public DetailAST findFieldInFrame(String name) {

public DetailAST findEnumConstInFrame(String name) {

single return point from method.

12)

> private boolean isShouldBeProcessed = true;

Please extend javadoc with more details why we need this field, and what is role of it.

thanks,

Roman Ivanov

​

[Vladislav Lis]

Hi Roman,

The latest commit

1) Done.
2) Done.
3) Done.
4) Done.
5) Done.

6)

> I hope this is the same

No, it's not. Methods can contain frames, such as IF, FOR and so on, which have to be checked.

7) Done.

8) Done.

9)
https://github.com/Vladlis/sevntu.checkstyle/commit/7e7ea660bdf8758e4fbca9a729f4451fe1def1e8#diff-0ab51992c3138d28f6d7cf9696b8a6f5R31

We cannot find neither a definition of a static method assertTrue() nor of a non-static, so we cannot say that foo9() can be declared static.

10) Done.
11) Done.
12) Done.

--

[Roman Ivanov]

Hi Vladislav,

https://github.com/Vladlis/sevntu.checkstyle/blob/7e7ea660bdf8758e4fbca9a729f4451fe1def1e8/sevntu-checks/src/main/java/com/github/sevntu/checkstyle/checks/design/StaticMethodCandidateCheck.java

1) 

> public int[] getRequiredTokens() {

I presume it should be a copy getAcceptableTokens, in other case you give use a chance to change token set to such a state that will case false-positives to appear. 

2)

 >5) Done.

I don not see it as DONE. boolean is still an argument of functions.

3)

Please send me a link to input that we found today in guava.

4)

> final DetailAST objCalledOn = getTheLeftmostIdent(methodCallAst);

move under "if (firstChild.getType() == TokenTypes.DOT) {" be lazy in calculations. 

Do not bother reader of your code with variables that  you do not use right-away.

5)

> final DetailAST field = findField(frame, identAst);

The same as "4)". Not all IF_ELSE branches need this variable, so move calculation of it to required branches only , as close as possible to first usage.

6)

> private static DetailAST findField(Frame startFrame, DetailAST identAst) {

make "while (frame != null && result == null) {" , and remove "break;"

if (field != null) {

result = field;

break;

} should be completely removed.

7)

> private static boolean isFrame(DetailAST ast) {

make it like "return stType == TokenTypes.VARIABLE_DEF || ......."

8)

>                 || field.getLineNo() == objCalledOn.getLineNo()

                && field.getColumnNo() < objCalledOn.getColumnNo()) {

indent second line to the right a bit to show priority of execution.

9)

> We cannot find neither a definition of a static method assertTrue() nor of a non-static, so we cannot say that foo9() can be declared static.

yes, from Checkstyle point of view assertTrueis non static method or method from parent class.

so it is should be enough to check only 

"return !hasNonStaticMethod"

thanks

Roman Ivanov

​

[Vladislav Lis]

Hi Roman,

The latest commit

1) I'm afraid I do not understand the meaning of getRequiredTokens() properly. Please, explain it to me privately.

2) My bad, done now.

3)

If you're talking about
https://github.com/google/guava/blob/50112d77b111cdc73a4e176abf547d2a3e6614d4/guava/src/com/google/common/util/concurrent/CycleDetectingLockFactory.java#L774

it is not a false-positive, because acquiredLocks is a class field
https://github.com/google/guava/blob/50112d77b111cdc73a4e176abf547d2a3e6614d4/guava/src/com/google/common/util/concurrent/CycleDetectingLockFactory.java#L496

nevertheless, here is the input for such a case, but when the field is a class member
https://github.com/Vladlis/sevntu.checkstyle/commit/a29c8c87a29c88932369ca99e2f9c76e28015cff#diff-0ab51992c3138d28f6d7cf9696b8a6f5R228

4) Done.

5) Done.

6) Done.

7) Done.

8) Done.

9)

I disagree, there are 3 cases:

    - we cannot find any method definition, so cannot claim, that the called method static or non-static,
      then hasNonStaticMethod == false and hasStaticMethod == false;
https://github.com/Vladlis/sevntu.checkstyle/commit/a29c8c87a29c88932369ca99e2f9c76e28015cff#diff-0ab51992c3138d28f6d7cf9696b8a6f5R23
https://github.com/Vladlis/sevntu.checkstyle/commit/a29c8c87a29c88932369ca99e2f9c76e28015cff#diff-0ab51992c3138d28f6d7cf9696b8a6f5R25
https://github.com/Vladlis/sevntu.checkstyle/commit/a29c8c87a29c88932369ca99e2f9c76e28015cff#diff-0ab51992c3138d28f6d7cf9696b8a6f5R31

    - we can find a non-static method definition, so it doesn't matter if we can find a static method definition as then we wouldn't be able to determine which one of these methods is called,

      then hasNonStaticMethod == true and hasStaticMethod doesn't matter;
https://github.com/Vladlis/sevntu.checkstyle/commit/a29c8c87a29c88932369ca99e2f9c76e28015cff#diff-0ab51992c3138d28f6d7cf9696b8a6f5R21
https://github.com/Vladlis/sevntu.checkstyle/commit/a29c8c87a29c88932369ca99e2f9c76e28015cff#diff-0ab51992c3138d28f6d7cf9696b8a6f5R151
https://github.com/Vladlis/sevntu.checkstyle/commit/a29c8c87a29c88932369ca99e2f9c76e28015cff#diff-0ab51992c3138d28f6d7cf9696b8a6f5R153

    - we can find only a static method definition, therefore we can claim that the static method is called,

      then hasNonStaticMethod == false and hasStaticMethod == true;
https://github.com/Vladlis/sevntu.checkstyle/commit/a29c8c87a29c88932369ca99e2f9c76e28015cff#diff-0ab51992c3138d28f6d7cf9696b8a6f5R62
https://github.com/Vladlis/sevntu.checkstyle/commit/a29c8c87a29c88932369ca99e2f9c76e28015cff#diff-0ab51992c3138d28f6d7cf9696b8a6f5R176
https://github.com/Vladlis/sevntu.checkstyle/commit/a29c8c87a29c88932369ca99e2f9c76e28015cff#diff-0ab51992c3138d28f6d7cf9696b8a6f5R188

that is why we need both variables.

thanks,

Vlad

--

[Roman Ivanov]

Hi Vladislav,

1)

> I'm afraid I do not understand the meaning of getRequiredTokens() properly. Please, explain it to me privately.

Call me, all details should be left at

https://github.com/checkstyle/checkstyle/issues/2346 , I described difference too many times, we need to document it.

2)

> it is not a false-positive

ok, I see.

3)

>  that the called method static or non-static,

yes, but that is enough for us to treat it as non-static. 

4)

> we can find only a static method definition, therefore we can claim that the static method is called

just make default boolean result as "TRUE" and find first non-static.

5) 

Please move getAcceptableTokens() above getDefaultTokens() to keep getDefault and getRequired  close to each other as they are the equal.

6)

from all methods that are processXXXXXX() please move code like :

"

currentFrame.addMethod(ast);

currentFrame.addChild(frame);

currentFrame = frame;

"

 to SWITCH-CASE. SWITCH will manage currentFrame member, all other supply frame/information for him.

7)

> private static boolean checkFrameExpressions(Frame frame)

I do not like indexes and extra boolean in expression, can we use http://docs.guava-libraries.googlecode.com/git/javadoc/com/google/common/collect/Iterables.html#all%28java.lang.Iterable,%20com.google.common.base.Predicate%29 ?

8)

> private static boolean checkTypes(Frame frame)

see "7)"

9)

> * Check whether a {@code static} method is called.

> private static boolean checkMethodCall(Frame frame, DetailAST methodCallAst)

 

Smth missed in Javadoc ? if not please name method as you describe it in javadoc - it is so simple

10)

> * Check whether the field is {@code static} or local.

> private static boolean checkField(Frame frame, DetailAST identAst)

see "9)"

11)

> result = frame.findEnumConstInFrame(fieldName);;

resolve ";;"

12)

> public DetailAST findFieldInFrame(String name)

> public DetailAST findEnumConstInFrame(String name) {

 

I think it is reasonable to use guava "find" method there to remove technical verbosity.

thanks,

Roman Ivanov

​

[Roman Ivanov]

Hi Vladislav,

> https://github.com/google/guava/pull/2185

13) please close it. 

14) Add a note to Check javadoc that such cases (calling by reflection private methods - https://docs.oracle.com/javase/8/docs/api/java/io/Serializable.html) is not supported and user need to suppress them. 

15) What about other PRs (base on your Check finding) to Guava ?

thanks

Roman Ivanov

​

[Vladislav Lis]

Hi Roman!

The latest commit

3)
> yes, but that is enough for us to treat it as non-static

It means, that default value should be FALSE in contrast to 4), but then we wouldn't be able to determine, when to stop searching for a static method even if we've found a non-static one

4)
> findStaticMethod

> just make default boolean result as "TRUE" and find first non-static

but if we do not find a non-static method and return TRUE by default, than this will be a false-positive, as we say then, that we have found a static method, even though we haven't
https://github.com/Vladlis/sevntu.checkstyle/commit/a29c8c87a29c88932369ca99e2f9c76e28015cff#diff-0ab51992c3138d28f6d7cf9696b8a6f5R23

5) Done.

6) Done.

7) 8) Done.

9) 10) Done.

11) Done.

12) Done.

13) Done.

14) Done.

15) In process.

Thanks,

Vlad

--

[Vladislav Lis]

Hi Roman,

The latest commit

Thanks

[Roman Ivanov]

Hi Vlad,

https://github.com/Vladlis/sevntu.checkstyle/blob/d9beb6838cc729685d4c8037c02f115cee85a4c5/sevntu-checks/src/main/java/com/github/sevntu/checkstyle/checks/design/StaticMethodCandidateCheck.java

1) 

Please create String option to check to allow user to skip validation of methods by name (default values need allow skip us false positives with Serializable methods)

2) 

> public void visitToken(final DetailAST ast) {

In this method please separate work with currentFrame and frame by "\n"

3)

> currentFrame.addEnumConst(ast);

move below to group currentFrame management

4) 

> private boolean checkFrame(Frame parentFrame) {

please order methods after all Override , base on first usage in class from top to bottom

5)

> 

isStaticCandidate = !frame.hasLiteralThis

&& isStaticCandidate move isStaticCandidate to be first in expression 6) >  if (isStaticCandidate) { log(frame.lineNo, MSG_KEY, frame.frameName); } else { isStaticCandidate = true; please explain in comment why isStaticCandidate = ! isStaticCandidate; 7) > private static Frame createFrameFromMethod(Frame parentFrame, DetailAST ast) { createMethodFrame 8)  > private static boolean isStaticMethodCalled(Frame frame, DetailAST methodCallAst) { isStaticMethod 9)  > private static boolean isStaticOrLocalField(Frame frame, DetailAST identAst) { What is local Field ? Why we need only local(current in class) ? any non static field is sign of skipping validation am I right ? 10)

>  private static DetailAST findField(Frame startFrame, DetailAST identAst) {

DetailAST result = null; private static Frame findFrameByName(Frame frame, String frameName) { Frame result = null;

As you start using Option, all methods that return null looks inconsistent. Please enforce Option in other methods.

11)

> return Iterables.find(fields, predicate, null);

Use method that return Option.

thanks,

Roman Ivanov

​

[Vladislav Lis]

Hi Roman,

The latest commit

1) Done.

2) Done.

3) Done.

4) Done.

5) Done.

6) It is old code which was needed, when isStaticCandidate was a class member.

7) Done.

8) Done.

9)
> What is local Field

it is local variable, renamed

10) Done.

11) Done.

Thanks,

Vlad

--

[Roman Ivanov]

Hi Vlad,

https://github.com/Vladlis/sevntu.checkstyle/blob/4cf214b606ac98789b5fb6c65b73c87b7cc01322/sevntu-checks/src/main/java/com/github/sevntu/checkstyle/checks/design/StaticMethodCandidateCheck.java

​1)

> By default its value: "readObject, writeObject,

By default its a private methods that class can have when it implement Serializable : "readObject, writeObject,

Make a link to Serializable javadoc.

2)

> // neither anonymous classes can't have static methods

remove "neither"

3)

> 

if (astType == TokenTypes.CLASS_DEF

&& !ScopeUtils.isOuterMostType(ast)

&& !ast.findFirstToken(TokenTypes.MODIFIERS)

.branchContains(TokenTypes.LITERAL_STATIC)) {

Just to make expression looks easy to read, make it as

            if (astType == TokenTypes.CLASS_DEF

                    && !ScopeUtils.isOuterMostType(ast)

                    && !hasStaticModifier(ast)) {

4)

>     private boolean checkFrame(Frame parentFrame) {

           ......

                    isStaticCandidate = isStaticCandidate

                            && !frame.hasLiteralThis

                            && checkFrameExpressions(frame)

                            && checkTypes(frame);

                    if (frame.isPrivateMethod) {

                        if (isStaticCandidate) {

                            log(frame.lineNo, MSG_KEY, frame.frameName);

Inconsistency in method naming. you take term "check" for method that do validation and logging of result- that is ok. But it is not ok to have the same prefix for method that return boolean.  

5)

> Check expressions in the given frame.

> private static boolean checkFrameExpressions(final Frame frame) {

Check for what ? please try to name method by what it is doing. See "4)"

7)

Fix all that  minors from above and please provide validation reports one more time , and links to PRs to Guava and Spring (no need to send all fixes to them, choose about 20-30 the most representative).

thanks,

Roman Ivanov

[Vladislav Lis]

Hi Roman,

The latest commit

1) Done.

2) Done.

3) Done.

4) Done.

5) Done.

7)

Guava:
https://github.com/google/guava/pull/2216
https://github.com/google/guava/pull/2215
https://github.com/google/guava/pull/2214
https://github.com/google/guava/pull/2213

Spring-framework has 258 violations - http://vladlis.github.io/reports/static_check/spring/ , and its hard to determine the most representative.

I've sent PR with fixes only for spring-core module:
https://github.com/spring-projects/spring-framework/pull/909

Thanks,

Vlad

--

[Vladislav Lis]

Hi Roman,

Please, see PR: https://github.com/sevntu-checkstyle/sevntu.checkstyle/pull/389

Thanks, Vlad

To unsubscribe from this group and all its topics, send an email to sevntu-checkstyle+unsubscribe@googlegroups.com.