Issue #454: fixed NPE in ConfusingConditionCheck

[rnveach]

Issue #454

Fixed NPE in getAmounOfCodeRowsInBlock.
Since the method is counting for blocks I set the return to 0 if there is no block. Please let me know if we should count lines even if there is no block.

[coveralls]

Coverage decreased (-0.001%) to 98.338% when pulling f8979a5 on rnveach:issue_454 into d876c9a on sevntu-checkstyle:master.

[romani]

Please let me know if we should count lines even if there is no block.

Please give me example of code.

[rnveach]

@romani
f8979a5#diff-1b93271e1dfd7a8eeec046006468c167R13

            if(flag);
            else;

another example:

            if(flag)
very.
long.
line();

Code and property define use for blocks, but I assume that means between {}s and these examples have no braces. It is just counting lines, not number of statements.
If we want it to also mean non-blocks, I can change it.

[romani]

It is just counting lines, not number of statements.

yes, that is all we need to do.
As screen size is certain amount of lines of code (not a statements). Idea is Check is to force user to write block code that fit a screen.

If there is no block (block is { ... }) it is not a target of this Check.

[romani]

Please provide:

1. stress testing report (on a lot of projects)
2. diff report (on a lot of projects, as no changes are expected)

[rnveach]

@romani
Diff report is not normally possible because this is a NPE and regression stops when an exception occurs. This is not the first time we ran into this.

I think its time we make some type of option in main checkstyle to prevent propagating the exception for these situations unless you know another way. Done correctly, the exception should be logged as a violation and show a difference. Maybe make changes for the diff tool to point them out.

I don't think it can be just an option in CLI since we are using JXR and maven-checkstyle in regression.

[rnveach]

Here is stress report (not diff):
http://rveach.no-ip.org/checkstyle/regression/reports/130/
No NPEs.

Config:

<module name="com.github.sevntu.checkstyle.checks.coding.ConfusingConditionCheck">
<property name="ignoreInnerIf" value="false"/>
<property name="ignoreSequentialIf" value="false"/>
<property name="ignoreNullCaseInIf" value="false"/>
<property name="ignoreThrowInElse" value="false"/>
</module>

[romani]

simple launch have too much violations to state that there are no regression.

Diff report is not normally possible because this is a NPE and regression stops when an exception occurs.

yes this is not typical . but diff is still possible, just demand a bit more manual actions.
Please

1. do first execution to get to know where Exception is present
2. exclude that folder or file from validation at all
   ....
   After few launches you will get clear diff report.

---

Please rebase this branch.

[coveralls]

Coverage decreased (-0.0008%) to 98.407% when pulling f97b668 on rnveach:issue_454 into d36dd3b on sevntu-checkstyle:master.

[rnveach]

diff is still possible, just demand a bit more manual actions.

The point isn't it is easy to work around it, the point is the report is now false as it shows no NPEs fixed.
It is also very easy to just forge the report and hide any projects/areas that report a difference since I am cutting out areas. Some of the files I am dropping could be the one few areas were a difference could be found in all our runs.
Fix in main checkstyle should also be an easy one as to not worry about this anymore.

---

Currently there are differences: http://rveach.no-ip.org/checkstyle/regression/reports/134/

It happens with code like: http://rveach.no-ip.org/checkstyle/regression/reports/134/apache-ant/xref/File1.html#L152

if (a) {
    ....
} else if (b) {
    ....
} else {
    ....
}

My new code is saying the first else has no lines because it isn't } else {.
The old code is not correct in counting either. firstBrace is set to the second if instead of the brace following it, and lastBrace is set to the final else instead of the brace right before it. Depending on actual brace positions, calculation could be wrong.

[coveralls]

Coverage increased (+0.003%) to 98.411% when pulling 14f47f0 on rnveach:issue_454 into d36dd3b on sevntu-checkstyle:master.

[rnveach]

new report on current state: http://rveach.no-ip.org/checkstyle/regression/reports/136/

http://rveach.no-ip.org/checkstyle/regression/reports/136/nbia-dcm4che-tools/index.html#A1

This is a difference because of incorrect calculation of else block in old code.

if (a) {
    ....
} else if (b) {
    ....
}

For the else, firstBrace is the second if statement and lastBrace is the brace right after the second if statement. It is incorrectly counting the number of lines as 0 instead of 2 in this linked case.
The new changes count the lines correct.

All the removals I looked at follow this pattern.
If you agree with everything, I will squash the commits.

[romani]

IF-ELSE-IF blocks can not be easily inverted , Check should skip them. Strange that is missed in javadoc.
Your last report is reasonable, please do squash.

[rnveach]

@romani Squashed.

can not be easily inverted

Why prevent them from being flagged, shouldn't it still be possible as long as second+ if isn't a negative condition too?

Check should skip them.

IF-ELSE-IF is not skipped.
Removals from last report are just because the ratio between the if and else (isRatioBetweenIfAndElseBlockSuitable) is not enough compared to multiplyFactorForElseBlocks to warrant a violation. Most likely there are other cases were a violation will be printed for IF-ELSE-IF, but this can be in a new issue.

[coveralls]

Coverage increased (+0.003%) to 98.411% when pulling 6af8d02 on rnveach:issue_454 into 4372fdc on sevntu-checkstyle:master.

[romani]

discussion could be continued at #531