sevntu-checkstyle / sevntu.checkstyle

Additional Checkstyle checks, that could be added as extension to EclipseCS plugin and maven-checkstyle-plugin, Sonar checkstyle plugin, extension for CheckStyle IDEA plugin.
http://sevntu-checkstyle.github.io/sevntu.checkstyle/
191 stars 146 forks source link

AvoidMultipleConditionsPerLine: new check #564

Open rnveach opened 7 years ago

rnveach commented 7 years ago

Taken from discussion at https://github.com/checkstyle/checkstyle/pull/4148#discussion_r109515706 :

Code coverage branches should be on their own line.

This will help with code coverage reporting. Reports currently use the same styling as the source code. This is fine until 1 line contains multiple branches. If not all coverage is hit, it makes it harder to determine which branch is failing as it doesn't single out the specific condition.

We should look into either creating this as a new check or see if this is just a specialized configuration of http://checkstyle.sourceforge.net/config_whitespace.html#OperatorWrap (always require new line before &&/||) .

The following cases should be reported as violations:

if (condition1 && condition2) {} // 1 violation
if (condition1 && (condition2 || condition3)) {} // 2 violations
if (condition1 &&
    (condition2 || condition3)) {} // 1 violation
for (int i = 0; condition1 && condition2; i++) {} // 1 violation
while (condition1 && condition2) {} // 1 violation
do {} while (condition1 && condition2); // 1 violation
return condition1 && condition2; // 1 violation
boolean test = condition1 && condition2; // 1 violation, confirmation of reporting of branches

return condition ? value1 : value2; // 1 violation, confirmation of reporting of branches

The following cases should not be report:

if (condition) {}
if (condition & condition2) {} // confirmation that it is seen as single branch because of single '&'
for (int i = 0; condition; i++) {}
while (condition) {}
do (condition) {}
return condition1
    && condition2;
boolean test = condition1
    && condition2;

if (condition1
    && condition2) {} // confirmation that it doesn't care where '&&' is on the line
if (condition1
    && condition2
    == value) {}
if (condition1
    && (condition2
        || condition3)) {}
for (int i = 0; condition1 && // confirmation that it doesn't care where '&&' is on the line
    condition2; i++) {}

return condition ? value1
    : value2;

@MEZk @romani Feel free to review and think up any weird situations we should examine and add.

MEZk commented 7 years ago

@rnveach One more example:

try {
    ...
}
catch (IOException | UnsupportedOperationException | Exception e) { // violation
    ...
}

try {
    ...
}
catch (IOException 
          | UnsupportedOperationException
          | Exception e) { // OK
    ...
}

The above specified example is a bit fanatic, that is why there should be a way to avoid that violation (for example, exclude the token from check's toke set). The example needs investigation. I'm not sure whether Cobertura and Jacoco can distinguish 'branches' of catch block.

(always require new line before &&/||)

Not only &&/||, but also ==, :, | are target tokens. There may be many nuances that cannot be covered by OperatorWrap.