maxvetrenko / checkstyle

Checkstyle is a development tool to help programmers write Java code that adheres to a coding standard. By default it supports the Sun Code Conventions, but is highly configurable. It can be invoked with an ANT task and a command line program.
http://checkstyle.sourceforge.net/
GNU Lesser General Public License v2.1
0 stars 0 forks source link

New Check: SeparatorWrap #10

Closed maxvetrenko closed 10 years ago

maxvetrenko commented 10 years ago

Cover the part of Google's rule with separators like '.' and ','.

romani commented 10 years ago

Separators and Operators: http://docs.oracle.com/javase/specs/jls/se8/html/jls-3.html#jls-3.11

please recheck that list of operators is full in OperatorWrapCheck.

rdiachenko commented 10 years ago
  • } catch (FooException
    • | BarException e) {} //bad wrapping

Why did you mark it as a bad case?

public void visitToken(DetailAST aAST)

  • {

I don't understand your logic. Let's discuss it

rdiachenko commented 10 years ago
  • *

    By default the check will check the following operators:

    • * {@link TokenTypes#TYPE_EXTENSION_AND},

SeparatorWrapCheck is for separators only, so it should contain only separators. We already have one for operators - OperatorWrapCheck. Please update it with missed operators.

rdiachenko commented 10 years ago

By default the check will check the following operators:

operators??

  • log(lineNo, colNo, "line.previous", text);
    • }
    • }
    • else if (currentLine.substring(colNo + text.length())
    • .trim().length() == 0)

Please format your code properly

if ((currentLine.substring(0, colNo).trim().length() == 0)) {

if (currentLine.substring(colNo + text.length())

  • .trim().length() == 0)

Move the logic to variables with proper names and use these variables in the condition statements. As for now it is hard to understand and debug these conditions

But the following wrapping example is not

Change to "But the following wrapping example is illegal"

maxvetrenko commented 10 years ago

operators??

Done

Please format your code properly

Don't understand. Checkstyle allow such formatting.

Move the logic to variables with proper name

Done

Change to "But the following wrapping example is illegal"

Done

rdiachenko commented 10 years ago

Don't understand. Checkstyle allow such formatting.

if (aAST.getType() == TokenTypes.COMMA) {
            if ((currentLine.substring(0, colNo).trim().length() == 0)) {
                log(lineNo, colNo, "line.previous", text);
            }
        }
 !!!!!!!!!!!!!!!!! REMOVE THIS BLANK LINE !!!!!!!!!!!!!!!!!!!!!!!!!
        else if (currentLine.substring(colNo + text.length())
                .trim().length() == 0)
        {
            log(lineNo, colNo, "line.new", text);
        }
rdiachenko commented 10 years ago

Move the logic to variables with proper name

Done

I don't see any changes

if (aAST.getType() == TokenTypes.COMMA) {
            if ((currentLine.substring(0, colNo).trim().length() == 0)) {
                log(lineNo, colNo, "line.previous", text);
            }
        }
else if (currentLine.substring(colNo + text.length())
                .trim().length() == 0)
        {
            log(lineNo, colNo, "line.new", text);
        }

Should be like this:

if (aAST.getType() == TokenTypes.COMMA) {
            String someVarWithProperName = currentLine.substring(0, colNo).trim();
            if (someVarWithProperName.length()  == 0) {
                log(lineNo, colNo, "line.previous", text);
            }
        }
else
{
String someVarWithProperName = currentLine.substring(colNo + text.length()).trim();
 if (someVarWithProperName.length() == 0)
        {
            log(lineNo, colNo, "line.new", text);
        }
maxvetrenko commented 10 years ago

REMOVE THIS BLANK LINE

Done

Should be like this:

Done

commit

rdiachenko commented 10 years ago
  • final static int LINE_BEGINING = 0;
    • final static int EMPTY_LINE_LENGTH = 0;

Move them back into the method. They are used only once in a single method. There is no reason to single them out as separate fields.

String sublineBefore String sublineAfter

Rename them as 'substringBeforeToken' and 'substringAfterToken'

Checks line wrapping for separators.

Change to "Checks line wrapping with separators."

Good wrapping example: For example the following wrapping is legal:

Change to "A good line wrapping example:"

Bad wrapping example: But the following wrapping example is illegal:

Change to "A bad line wrapping example:"

maxvetrenko commented 10 years ago

Move them back into the method. They are used only once in a single method. There is no reason to single them out as separate fields.

Done

Rename them as 'substringBeforeToken' and 'substringAfterToken'

Done

Change to "Checks line wrapping with separators."

Done

Change to "A good line wrapping example:"

Done

Change to "A bad line wrapping example:"

Done

rdiachenko commented 10 years ago

Change to "Checks line wrapping with separators."

Not done. Use Ctrl+F to look for all the entrances

maxvetrenko commented 10 years ago

Not done. Use Ctrl+F to look for all the entrances

Done

romani commented 10 years ago

ok to PR.