We've done a few fixes in JavaFX trying to reduce the amount of
warnings, and there was some discussion as to the value of fixing
certain warnings. Some of these are purely aesthetic (like using the
diamond operator) but others can hide current or future programming
problems.
I thought I'd post why I think fixing certain warnings is important for
the continued health of a large code base.
- Unnecessary casts (on Objects)
Every cast in a code base is a potential place where a
ClassCastException may lurk. When a cast is unnecessary, it may seem
harmless to leave it in. However, as a code base grows or is
refactored, a harmless cast hides a potential compilation problem which
can become a runtime ClassCastException. Java developers rely a lot on
the compiler helping them when they do small refactorings to point out
places where a type no longer matches after a change. Take the code below:
private Number n;
private Float f = (Float) n; // warning unnecessary cast
Changing `n` to a more specific type (let's say `Double`) will not
generate an error for `f`. The only thing that happens is that the cast
is no longer deemed unnecessary and will do an actual cast (resulting in
CCE at runtime) -- as there is no error reported, the developer may
easily miss this.
- Unnecessary casts (on primitives)
A cast on a primitive that is not currently required can become a place
where precision is lost without warning when a refactoring occurs.
private int x;
private int y = (int) x; // warning unnecessary cast
A refactoring that changes `x` to a type like `long` would go unnoticed
by the compiler. The version without the unnecessary cast would emit an
error.
- Unnecessary declared checked exceptions
When a checked exception is declared that is never thrown from the
underlying code, it is good to remove these to prevent them from
propagating up the call chain. Sometimes code bases have a checked
exception in some deeply nested function that is not actually thrown,
leading to perhaps top level public API to declare this exception. When
the problem is discovered, the exception could be removed from dozens of
places and could even affect public API (which now throws an exception
that it can't actually throw but can't be removed as it is was already
made public).
- Missing @Override annotations
Adding these everywhere makes extending super types and refactoring
safer. A sub type may declare a method that currently does not override
any of its super types. If a super type is changed and a method is added
that has a signature that is compatible with one that is already present
in a sub type, the compiler will emit a warning that it is missing the
override annotation alerting you of a potential problem (assuming the
code base is relatively warning free, or it may get lost in the noise).
Conversely, removing or changing a currently overriden method would
result in errors if sub types declared their overriden methods with the
override annotation, alerting you that this functionality was perhaps
more broadly used than anticipated. Without the annotation, the method
in the sub type would simply change to a non-overriden version without
any further warning.
- Avoiding indirect access to static methods and fields
Code that accesses static members via `this` or indirectly via a sub
type that didn't declare it, runs the risk of that access being diverted
to a new member if one is introduced with the same name without a
compiler error:
class A { static final int ONE = 1; }
class B { }
class C extends B {
int one = ONE; // unqualified access to A.ONE
}
A "harmless" constant is introduced in B:
class B { static final int ONE = 0x10000; } // 1 in 16-bit
fixed point
No error occurs, and class C is broken.
A further reason to not allow indirect accesses to static members is
that it may hide problems where an instance wasn't needed at all (if
using `this.ONE` for example).
- Using raw types
With the introduction of generic types in Java 1.5, use of raw types
should be considered something to be always avoided. Using raw types
will require casts in places that the compiler could have inferred or
checked for you if you had used a generic type. Not using a generic type
is basically risking a ClassCastException in those places (it was one of
the most common exceptions in the pre-1.5 days). Further more, the
compiler can't help you during refactorings. If a generic declared List
is changed to contain a different type, there will be compiler errors in
all the places you interact with the List. Not so for a raw List;
failing to fix all the casts will pay a runtime price.
- Unused fields / variables / parameters
All of these may point to a potential problem. It could be that the
author mistyped something and meant to use one of the unused values; it
could be that a parameter is genuinely unused and should be removed
(triggering a potential cascade in callers that may be doing unnecessary
actions to "provide" this unneeded information); it could be that a
field is accessed by reflection (in which case that would have meritted
a BIG warning comment and `SuppressWarnings`); whatever the case, it is
confusing to future maintainers, may involve unnecessary memory use or
unnecessary calculations (setting but never reading a value could
involve a big calculation that may be completely unnecessary).
--John
