Removing false positives

Owned by Former user (Deleted)
Last updated: Jun 28, 2006Version comment
2 min read

Some notes on the configuration of the automated code review -

By Antranig Basman

Comments

Just the sort of email that improves the automated code review service.

The email

This has been a highly valuable study - the XRef source is very useful,
and the review has turned up many useful results (probably chief
amongst these is the prevalence of empty catch blocks), however there
are a number of false positives that should be removed from the
configuration next time the inspection is run.

Referring to http://bug.sakaiproject.org/confluence/display/QA/Automated+Code+Review

I think the following class of errors are worthless and should be
disabled from the configuration:

i) The return value of non-void method "XXXX" is not used

This almost never represents any kind of programming error. The days
when programs signalled "vital conditions" via return values went
out in the mid 90s...

ii) The constructor of the direct super class ("super()") should be
called first

This is simply incorrect - it will be implicitly called first if
not mentioned, there is no need to invoke it directly.

iii) Document empty constructor

Really this is never necessary.

iv) "All methods are static"

This is a perfectly valid design strategy.

Removing these three cases will remove virtually all the "noise" from
the "Errors" pages for the modules.

The following are of neutral use:

i) "Do not use == to compare objects" - whilst this possibly
represents a poor design practice, it is in fact in Java often a
very valid strategy assuming it is used with correct understanding
of the environment. I note for example that a lot of these errors
occur in the gradebook for comparison with floating point 0.0 which
is probably valid as a "sentinel value".

The following are highly valuable:

i) empty catch blocks
ii) Objects marked as "Serializable" which are not
iii) Duplicate code detection (actually, this is priceless)

The links seem to be broken to the Xref from the "Style Check"
pages (e.g. http://mercator.ic.uva.nl/sakai_review/006/pmd/sam/pmd_details.html)

I haven't been in detail through all of the "Style Check"
guidelines, but most of them seem fairly well motivated (Cyclomatic
complexity (although I would put the warning threshold here MUCH
higher), overlong methods, non-negated conditions, instantiating
Boolean objects, overridable method call, avoiding use of toUpper()
&c.)