sevntu-checkstyle / sevntu.checkstyle Goto Github PK

Trying to parse the following java class ForbidAnnotationCheck will fail with NPE:

import java.awt.Component;

@org.springframework.stereotype.Component
public class ForbidAnnotationInput2 extends Component {
private static final long serialVersionUID = 1L;

}

Workaround:

import org.springframework.stereotype.Component;

@component
public class ForbidAnnotationInput2 extends java.awt.Component {
private static final long serialVersionUID = 1L;

}

(in progress by Sergey Burtsev) check to standardize condition expressions. Safe (in C++ approach) IF conditions. Example : "if (varialble == null)" ==> "if (null == varialble)"
In Java there is no posibility to make ocasionally "=" instead of "==". so We need to force developers to write "if (varialble == null)".

http://sourceforge.net/tracker/?func=detail&aid=3004587&group_id=29721&atid=397081
https://sourceforge.net/tracker/?func=detail&aid=3103881&group_id=29721&atid=397081

Please take a look at existing AbstractClassNameCheck class.

It is necessary to do as an option:
Check that classes with name "AbstractXYZ" has 'abstract' modifier.

Option should be disabled by default.

UTs should cover all additional functionality.
How to contribute: http://checkstyle.sourceforge.net/contributing.html

ProducerExtendsConsumerSuper.
Joshua Bloch "Effective java (2nd edition)" Item 28 page 138:
"
always use Comparable<? super T> in preference
to Comparable<T>. The same is true of comparators, so you should always
use Comparator<? super T> in preference to Comparator<T>.
"
as options:

• List of class names that are always producers. Default values for "Comparator|Comparable".
• list of class names that are always customers.

http://sourceforge.net/p/checkstyle/feature-requests/581/

1. Current dependency for Checkstyle is 5.4-SNAPSHOT - change it to 5.3
2. Provide Howto on wiki for building test-jar for it, including verbose checkouting and changing branch to 5.3 release
3. Current dependency for eclipse-cs is 5.3-SNAPSHOT - change it to 5.3
4. Provide Howto on wiki for bulding it, including verbose checkouting and changing branch to 5.3 release

ForbidWildcardAsReturnType.

Joshua Bloch, "Effective Java (2nd edition)" Item 28: page 137 :
"Do not use wildcard types as return
types. Rather than providing additional flexibility for your users, it would force
them to use wildcard types in client code.
Properly used, wildcard types are nearly invisible to users of a class. They
cause methods to accept the parameters they should accept and reject those they
should reject. If the user of a class has to think about wildcard types, there is
probably something wrong with the class’s API."

Options:

• check public method (boolean) (default value = true)
• check protected method (boolean) (default value = true)
• check protected-package methods (boolean) (default value = true)
• check private methods (boolean) (default value = false)
• allow Wildcard with "super" (boolean). example: "? super T" (default value = false)
• allow Wildcard with "extends" (boolean). example: "? extends T" (default value = false)
• ignore list for Class names (regexp), example "Comparable<? super T>". (default value = "Comparable|Comparator")

http://sourceforge.net/p/checkstyle/feature-requests/580/

http://today.java.net/article/2006/04/04/exception-handling-antipatterns#ignoringInterruptedException

just catch ignoring of exception by regexp pattern for Exception name. Default value is "InterruptedException" .
In Russian : http://habrahabr.ru/post/164487/ and first comment.

It might be reasonable to create general check NeverIgnoreException and by default setup it for InterruptedException. To allow developer to write any code in catch, and we can not check whether it is correct code or not - it all up to developer.

Here you may found instructions about custom twitter checkstyle checks:
https://github.com/twitter/commons/tree/master/build-support/checkstyle

Investigate:

1. Since they are open sourced under Apache2 license should we somehow use them? Maybe depend on them (they are packaged as separate ivy-module, that is essentially a maven module and deployed here - http://maven.twttr.com/com/twitter/common/checkstyle/) and provide xmls for easy use of them from the eclipse-cs plugin?
2. Their way of adding them to the checkstyle during checking the source. It seems that they are used simply adding jars with them to the classpath during checkstyle run. It seems that they use it very easily in their checkstyle ant task. If so it should be possible to use our checks in checkstyle ant task two. Provide the tutorial, how to use them with the manually downloaded sevntu-checks.jar (from the soon to be published our maven repository) and how to use them with the help of ivy.

https://github.com/sevntu-checkstyle/sevntu.checkstyle/blob/master/sevntu-checks/src/main/java/com/github/sevntu/checkstyle/checks/coding/AvoidConstantsInInterfacesCheck.java
should be removed as it is implemented incorrectly(it should check that interface contains only constant declarations) and already exists in standard Checkstyle:
http://checkstyle.sourceforge.net/config_design.html#InterfaceIsType

There is anti-pattern - http://today.java.net/article/2006/04/04/exception-handling-antipatterns#logAndThrow

so we need to detect logging and re-throwing exception:

catch (NoSuchMethodException e) {
  LOG.error("Blah", e);
  throw e;
}

but we need to have an option to ignore cases that do logging but without exception information, so it possible that before throw we would like to print a lot of useful internal information for investigation

catch (NoSuchMethodException e) {
  LOG.error("Some information that is known only at this scope", someField);
  throw e;
}

Fix check and transfer UTs for it to sevntu-checks-tests project.

SimpleAccesorNameNotationCheck - check only direct fields and setter and getter. Options: 1) member prefix "m_" 2) check only simple getter/setter XXXType getXXXName(); void setXXXName(XXXType)

Partial implementation (does not work as expected): https://github.com/sevntu-checkstyle/sevntu.checkstyle/blob/master/sevntu-checks/src/main/java/com/github/sevntu/checkstyle/checks/coding/SimpleAccesorNameNotationCheck.java

https://sourceforge.net/tracker/?func=detail&aid=3298879&group_id=29721&atid=397081

Add opportunity to checkstyle or to Eclipse Checkstyle Plug-in add to Ignore List packages and classes by some pattern or by name.

Check Spring annotation @Autowired method to be placed after fields/constructor declaration.
@Autowired fields to be placed after constant and before ordinary fields.
Provide Unit Tests for created checkers.

• Correct finalize() declaration. It is not a very simple and straight forward check, need investigation.

rules are here:
http://www.ibm.com/developerworks/ru/library/j-jtp06294/#2.3

we could check all except for calling finalize from user code (3rd in list of validations).

This Check detects 3 most common cases of incorrect finalize() method implementation:

//negates effect of superclass finalize
protected void finalize() { }
protected void finalize() { doSomething(); }
//useless (or worse) finalize
protected void finalize() { super.finalize(); }
//public finalize
public void finalize() { try { doSomething(); } finally { super.finalize() } }

AbbreviationAsWordInNameChecks should ignore constants declaration in interfaces if
ignoreFinal or ignoreStatic is TRUE. As all the variables in an interface are implicitly static finals.

public interface IWorkbenchComponent  extends IPerspective {

    String TITLE = "title"; //Unexpected WARNING HERE !!!

    void dispose();
}

from PMD http://pmd.sourceforge.net/pmd-5.0.0/rules/index.html:
ConfusingTernary: Avoid negation within an "if" expression with an "else" clause. For example, rephrase: if (x != y) diff(); else same();as: if (x == y) same(); else diff();Most "if (x != y)" cases without an "else" are often return cases, so consistent use of this rule makes the code easier to read. Also, this resolves trivial ordering problems, suchas "does the error case go first?" or "does the common case go first?".

we just give idea from PMD, we have the constant problem in code - so we need to automate it a bit at least as in PMD.
We need to check even short form of conditions "?(a != b)c=1:c=2;"

Almost all unit tests fail on windows due to issues with path separators.

• (in PROGRESS: Vadim Panasjuk) return in final block. To support a case, and show a cases http://stackoverflow.com/a/65362.

Should be tested again as it buggy (throw exception, but I do not remember exact use case):

https://github.com/sevntu-checkstyle/sevntu.checkstyle/blob/master/sevntu-checks/src/main/java/com/github/sevntu/checkstyle/checks/coding/RedundantReturnCheck.java

https://sourceforge.net/tracker/?func=detail&aid=3170845&group_id=29721&atid=397081

Make an exceptions, as options, for:

• field declaration.
• method declaration.

ChildBlockLengthCheck doesn`t provide info about real size of 'bad' block (in lines)

http://checkstyle.sourceforge.net/config_coding.html#SuperClone

options to detect: final classes, call of constructors in clone.
Any other improvements to SuperClone?

Best practices:
http://www.javaworld.com/javaworld/jw-01-1999/jw-01-object.html?page=2

Bug in public methods in c-tor.
https://github.com/romani/sevntu.checkstyle/blob/master/sevntu-checks/src/main/java/com/github/sevntu/checkstyle/checks/coding/OverridableMethodInConstructorCheck.java

public ULCWorkbenchPartSite(Object input) {
    super(window, part, descriptor);
    globalActions = new ActionsHelper(part);

    init(); // warning here is corrects
    initPart(part, input); // warning here is not correct 
}
   public init() {
   ///....
   }
   private initPart() {
   ///....
   }

one case is valid another is not:
init();
initPart(part, input); // no warning should be here

Look like you have problem with name recognition as you treat them as the same methods.

Assigned to: Hena Shah (https://groups.google.com/forum/#!topic/checkstyle/WOEyPxlTwLI).

ValueOfInsteadOfNewCheck.
Attempt to guide user to use Integer.valueOf(X) in favor of new Integer(X).

Check could define an validation option of each Boxing candidate:
Byte - http://grepcode.com/file/repository.grepcode.com/java/root/jdk/openjdk/6-b14/java/lang/Byte.java#Byte.valueOf%28byte%29
Long - http://grepcode.com/file/repository.grepcode.com/java/root/jdk/openjdk/6-b14/java/lang/Long.java#Long.valueOf%28long%29
Integer - http://grepcode.com/file/repository.grepcode.com/java/root/jdk/openjdk/6-b14/java/lang/Integer.java#Integer.valueOf%28int%29
Short - http://grepcode.com/file/repository.grepcode.com/java/root/jdk/openjdk/6-b14/java/lang/Short.java#Short.valueOf%28short%29
Character - http://grepcode.com/file/repository.grepcode.com/java/root/jdk/openjdk/6-b14/java/lang/Character.java#Character.valueOf%28char%29

Ideas/details: http://stackoverflow.com/questions/2974561/new-integer-vs-valueof .

Create the shell script which can display a Checkstyle AST tree for custom Java file.

It will be great to execute in shell command such as 'ast .../File.java' to see the appropriate synthax tree in Checkstyle GUI

This can be done using shell aliases.

info: http://checkstyle.sourceforge.net/writingchecks.html

Two check was implemented:

• UnnecessaryParenthesesFeature
• HighlightAllDuplicates

Merge patches from:
https://github.com/balakhonov/patches/tree/master/patches
to our repository

Check order of declaration for class(main or internal):
fields,
Annotated fields,
c-tor,
abstract methods,
Annotated methods(Autowired, Override,...),
methods,
internal classes.

As sub task:
Check nested/internal classes to be declared at the bottom of the class (after all methods/fields declaration).

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

proposed name for macros ###filed_ann_class###

2. Setter/Geter (base on bean notation) , notation detection could be 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 getX(){}
public int setX(){}
public int getY(){}
public int setY(){}

3. include coverage of nested interfaces, nested enumarations
   old :" Options that are "true" by default ignoreInnerInterfaces ignoreInnerEnums"
   new create new macros: InnerInterfaces InnerEnums to help manage it location.

public class MyClass {

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

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

4. include coverage nested classes in methods
   Options that are "true" by default
   ignoreIneerClassesInMethods

Example of bad code:

private methodX (... ) {

class NameC{ .... }
....
}

}// end of methods declaration

5. Add support for escape symbol "" in regexp (it is required to have in regexp string usage of "(" and ")" symbols - for example describe in format location of "main()" method)
   example of usage in: "........main()...."
   Attention: see Baratali explanation for this feature.

ATTENTION: example of custom order declration format is here https://groups.google.com/d/msg/sevntu-checkstyle/_nOVtWDxPJo/sBb8s2D4lTwJ

Code:

@OverRide
public Object convertToObject(String newString, Object previousValue)
throws DataTypeConversionException {
if (BigDecimal.class.equals(type)) { // fix for #279
DecimalFormat format = (DecimalFormat) NumberFormat.getNumberInstance(getLocale());
format.setParseBigDecimal(true);
try {
return format.parse(newString);
} catch (ParseException e) {
ErrorObject errorCode = new ErrorObject(ErrorCodes.ERROR_CODE_BAD_FORMAT, newString);
throw new DataTypeConversionException("Could not be converted to BigDecimal", e, errorCode); //// INCORRECT WARNING HERE!!
}
}
return convertType(super.convertToObject(newString, previousValue));
}

Please update Uts as support this case.

if return result for the method is Collection or array, show warning when it is trying to return null instead of empty collection or empty array.

Returning Null is error prone as developers forget to review method implementation or Java doc for nuances of Null meaning.

We need a check that forbids nested ternary operators. Example:

int smth = (a == b) ? -1 : (c == d) ? 0 : 1; 

This is really a bad practice.. This line should be refactored to:

int k;
if (a == b) {
    k = -1;
} else {
    if (c == d) {
    k = 0;
    }
    else {
        k = 1;
    }
}

Check should have a single option named 'ignore final' which allows to ignore statements such as:

final int smth = (a == b) ? -1 : (c == d) ? 0 : 1; 

in ctors, because in c-tors there is only one way to deal with such kind of logical operations on final variable.

It will not eliminate all false-positives, but will give a reasonably acceptable workaround - consider annotation @OverRide when applying this check to method names.

Sources except eclipse-plugin-plugin reside just in root of the repository. Since we now have two connected projects - eclipse-plugin-plugin and a fork of checkstyle changes from which we are trying to put upstream, we need to mode this second project in some folder in the root of the repo.
The investigation how it can be done with git is needed.

Private fields that can be converted to locals. Same as in PMD: SingularField

https://sourceforge.net/tracker/?func=detail&aid=3222783&group_id=29721&atid=397081
http://pmd.sourceforge.net/xref/net/sourceforge/pmd/rules/design/SingularField.html

ChildBlockLengthCheck doesn`t provide info about real size of 'bad' block (in lines)

IllegalCatchCheck is already transferred to IllegalCatchWithIgnoreCheck in eclipse plugin and works. Transfer from other sources tests for them.

I added a check to ensure that inner classes placed lower than methods, it works wrong in this particular case:

    class A {
        public Object createB() {
            class B {

            }
            return new B();
        }

        void otherMethod() {

        }
    }

ie classes that declared inside methods should be skipped from check.

Private inner types(Classes, Interfaces, enums) should NOT be placed as return type for public method. We need to ignore such types if they are extends or implements smth (usually public interface) so clients of such class could keep them by public interface.

https://sourceforge.net/tracker/?func=detail&aid=3103873&group_id=29721&atid=397081

Consider user Map.Entry for whole map iteration instead of
getKeys() and then get value by key.
check could simply check while/for for such case. This check will not find all
weird cases but will help significantly with major cases, that are done as C++ habit, refactoring, .....

http://sourceforge.net/p/checkstyle/feature-requests/586/

from Check JavaDoc:
This check can help you to write the whole for-each map iteration more correctly:
If you iterate over a map using map.keySet() or map.entrySet(), but your code uses only map values, Check will propose you to use map.values() instead of map.keySet() or map.entrySet(). Replacing map.keySet() or map.entrySet() with map.values() for such cases can a bit improve an iteration performance.
Bad:

for (Map.Entry; entry : map.entrySet()){
   System.out.println(entry.getValue());
}
for (String key : map.keySet(){
   System.out.println(map.get(key));
}

Good:

for (String value : map.values()){
   System.out.println(value);
}

If you iterate over a map using map.entrySet(), but never call entry.getValue(), Check will propose you to use map.keySet() instead of map.entrySet(). to iterate over map keys only.
Bad:

for (Map.Entry entry : map.entrySet()){
   System.out.println(entry.getKey());
}

Good:

for (String key : map.keySet()){
   System.out.println(key);
}

If you iterate over a map with map.keySet() and use both keys and values, check will propose you to use map.entrySet() to improve an iteration performance by avoiding search operations inside a map. For this case, iteration can significantly grow up a performance.
Bad:

for (String key : map.keySet()){
   System.out.println(key + "  " + map.get(key));
}

Good:

for (Map.Entry entry : map.entrySet()){
   System.out.println(entry.getValue() + "   " + entry.getKey());
}

Setup ignore for particular check base on package name or direrctory name.

It will be useful to for packages that are created by code generation of copy-paste from other projects.

It is necessary usually to switch off all validation of the CheckStyle base on:

• @deprecated (or any other annotation)
• File name (for example XXXXTest.java )
• base on package name ( example: com.mycompany.badcode )

Check Spring annotation @Autowired method to be placed after fields/constructor declaration.
@Autowired fields to be placed after constant and before ordinary fields.
Provide Unit Tests for created checkers.

Cases that VariableDeclarationUsageDistanceCheck have to support
1.

       int mm = Integer.parseInt(time.substring(div + 1).trim());

        Calendar cal = Calendar.getInstance();
        cal.setTimeInMillis(timeNow);
        cal.set(Calendar.SECOND, 0);
        cal.set(Calendar.MILLISECOND, 0);
        cal.set(Calendar.HOUR_OF_DAY, hh);
        cal.set(Calendar.MINUTE, mm);

        Option srcDdlFile = OptionBuilder.create("f");
        Option logDdlFile = OptionBuilder.create("o");
        Option help = OptionBuilder.create("h");

        Options options = new Options();
        options.something();
        options.addOption(srcDdlFile, logDdlFile, help);

https://sourceforge.net/tracker/index.php?func=detail&aid=3298881&group_id=29721&atid=397081#

Improve JavadocMethod check to cover only abstract methods, or only inheritable methods, or only final methods etc.

http://sourceforge.net/tracker/index.php?func=detail&aid=2959656&group_id=29721&atid=397081

Additional request is to - Force users to write java doc for Interfaces/Abstract classes only.
http://sourceforge.net/tracker/?func=detail&aid=2959656&group_id=29721&atid=397081
http://stackoverflow.com/questions/13271326/checkstyle-check-only-method-in-interface

UPDATE: Checkstyle supports
https://checkstyle.org/config_filters.html#SuppressionXpathSingleFilter from 8.18
by means of fancy xpath expression we can exclude all not required violations while Check can not support it more easy way.

Deprecated annotation and tag in java doc have to follow with @link to new realization. To answer developer what should be used instead. Consider existing checks for example http://checkstyle.sourceforge.net/config_javadoc.html, Example of @link "http://docs.oracle.com/javase/1.5.0/docs/guide/javadoc/deprecation/deprecation.html

@deprecated As of release 1.3, replaced by {@link #getPreferredSize()}
"

Force Tests to follow naming convention.
Ground: maven do not run tests that are not following naming convention (http://stackoverflow.com/questions/6178583/maven-does-not-find-junit-tests-to-run).
Requirement: Force classes having unit tests to follow naming convention - class should be named "Test_", "_Test", "_TestCase" or "_IT" (the latter is for integration tests)
Criteria: Class is considered to have tests if it has methods annotated with "@test" or it extends other class that follows naming convention.

Already done :), see impressive answer
http://http://stackoverflow.com/questions/10636563/checkstyle-rule-for-annotations-must-be-on-separate-line

if Author did not do this himself, I sent him request, we will copy this code to us ourself

abstract class should not have "main()" method - as it is mislead that it is completed implementation and ready to work.