Collect: Disable certain FindBugs warnings

Created on 15 Mar 2019 · 6Comments · Source: getodk/collect

(Developer-facing only.)

Certain FindBugs warnings seem likely (in my opinion) to cost more than they will benefit us. These are the ones I feel most confident about disabling:

SBSC_USE_STRINGBUFFER_CONCATENATION: This prohibits using + on strings in a loop. Rationale for disabling: This warning does not detect a likely defect; it only forces premature optimization and makes our code slightly harder to read. Benchmarks show that you have to loop a large number of times before the speed difference is significant (e.g. 50,000+ times before you see a 50% speedup). We rarely use + in a loop that runs such a large number of times that we would notice any improvement.
INT_BAD_REM_BY_1: This prohibits using the % operator with a modulus that can be determined to be 1. Rationale for disabling: Because this prevents % with a constant expression that might ever evaluate to 1, it can make using and adjusting constants more costly. SpotBugs also considers this a low-priority warning.
DB_DUPLICATE_SWITCH_CLAUSES: This prohibits a switch statement from having two cases that contain the same body. Rationale for disabling: It is a conceivable state of affairs for code to pass through a state in which two cases contain the same body, as the code evolves; when a change is needed in the future, the change might not apply to both cases. Collapsing the cases is often be a tidy thing to do, but being forced to always collapse the cases is a maintenance cost. SpotBugs also considers this a low-priority warning.
NS_NON_SHORT_CIRCUIT: This prohibits the use of | and & in certain if-conditions and assignment expressions (because it might be a typo for || or &&). Rationale for disabling: This is the low-confidence version of NS_DANGEROUS_NON_SHORT_CIRCUIT, which is better at detecting a problem; we should enable that one and disable this one.
RCN_REDUNDANT_NULLCHECK_OF_NONNULL_VALUE: This prohibits null checks against anything that the compiler can determine to be always non-null. Rationale for disabling: This is unlikely to indicate a programming defect; it is just defensive coding. Further, the detection behaviour depends on optimization logic, which means that predicting whether this warning will occur entails accurately replicating the optimization logic in your head, a potentially large cognitive cost. SpotBugs also considers this a low-priority warning.