Fix Issue #184 #2295
Fix Issue #184 #2295
Conversation
The logic conditionals were entangled in an invalid way that produced nonsense debug log messages. Simplified the logic and reduced the logic branches to make the operation direct and thus fix the problem.
| @@ -190,15 +190,13 @@ public final void reportBug(@Nonnull BugInstance bugInstance) { | |||
| } | |||
| int priority = bugInstance.getPriority(); | |||
| int bugRank = bugInstance.getBugRank(); | |||
| if (priority <= priorityThreshold && bugRank <= rankThreshold) { | |||
| doReportBug(bugInstance); | |||
| if (priority > priorityThreshold) { | |||
There was a problem hiding this comment.
This logic seems wrong. If the priority is higher than the threshold, it should be reported, not filtered, right?
Seems to me that the correct fix is simply to swap the first condition so that it reports the bug if either priority or bugRank is greater than the threshold, and leave the rest basically the same.
There was a problem hiding this comment.
Threshold for both priority and rank operates with a "lower value is more important".
Look at the original code
if (priority <= priorityThreshold && bugRank <= rankThreshold) {
doReportBug(bugInstance);
}
I think just this comment alone illustrates why the original logic was overly complex and unnecessary. Even experienced reviewers get it wrong.
There was a problem hiding this comment.
I could have effected the same fix just by reversing the logic in the else clause from <= to > but after reading this code the current form is easier to follow and reduced the number of comparisons to exactly 2 with 3 logical outcomes.
There was a problem hiding this comment.
You may need to look at the issue reported. The problem is not with the reporting but with the invalid debug logging. What was happening before is that when the reporting was skipped due to priority the debug log would instead report skipping due to rankThreshold with nonsense conditions. The opposite would also do the same, a skip due to rank would debug log that skip due to priority with invalid conditions.
Obviously if the original logic was flawed this would have been fixed a long time ago, it was only the debug logging that was confusing and this was never addressed.
In my opinion this makes the intended logic clear and unambiguous.
There was a problem hiding this comment.
Perhaps a unit test would help restore confidence in this logic and eliminate any questions. There were no unit tests I could find that validated logged messages but it may be useful to at least make a unit test that proves doReportBug is called based on the rules of this logic.
There was a problem hiding this comment.
This logic seems wrong. If the priority is higher than the threshold, it should be reported, not filtered, right?
Seems to me that the correct fix is simply to swap the first condition so that it reports the bug if either priority or bugRank is greater than the threshold, and leave the rest basically the same.
I am not going to make the changes due to the reasons stated above. The code is correct as it stands and I expect this would be approved now. I added the unit test to prove that the intended filtering was working the same way before and after my changes.
There was a problem hiding this comment.
@Alex-Vol-Amz Think I'm following the logic error. The problem was that both were considered for the original first check.
priority <= priorityThreshold && bugRank <= rankThreshold
Then it was checking this again
priority <= priorityThreshold
If it didn't hit that then it assumed rank threshold to log. Problem as I see original code then just to clarify my understanding is that priority or bug ran could be > and such the only path was for rank threshold to write the debug line.
I agree it was overly complex as I had to think too hard to even respond here. Its far easier in new state as follows.
Write debug...previously this was a bug, if it was > then it was never logged, it fell through to rankThreshold incorrectly.
priority > priorityThreshold
Write debug...
bugRank > rankThreshold
Otherwise....reportBug
There was a problem hiding this comment.
Exactly, which is now also covered by unit test so we know for sure it will stay fixed in this new form.
I wish the code was amenable to actually mock the logger so one could even verify that the debug is actually logged and correct, but given the direct approach followed here it becomes simple binary logic.
There was a problem hiding this comment.
Thanks @Alex-Vol-Amz I'm good with the changes. Going to merge it in now.
This unit test serves as proof that the reportBug function is operating as expected with respect to reporting based on priority and rank threshold values. As a bonus this test also verifies that the relaxed mode will override any rank or priority and always report bugs found.
|
Performed the execution of the new unit test with and without the main code change. The result was the same passing test as expected since the intention of the code change was to fix the debug log, not change the reporting behavior. |
hazendaz
dismissed
ThrawnCA’s stale review
Satisfactory information from contributor on the issue, full suite of test cases, along with second review here. All seems ok and far easier to read. It was a problem with logging.
|
@Alex-Vol-Amz All in all, satisfied with the change as far more readable and avoids having to think to hard on what the processing was doing. Thanks for going that extra mile generating tests for the changes as that helps considerably with the effort. |
The logic conditionals were entangled in an invalid way that produced nonsense debug log messages. Simplified the logic and reduced the logic branches to make the operation direct and thus fix the problem.
Make sure these boxes are checked before submitting your PR -- thank you!
CHANGELOG.mdif you have changed SpotBugs code