Dodgy code - rules provided by FindBugs
Questionable cast to abstract collection
This code casts a Collection to an abstract collection (such as List, Set, or Map).
Ensure that you are guaranteed that the object is of the type you are casting to.
If all you need is to be able to iterate through a collection, you don't need to cast it to a Set or List.
Questionable cast to concrete collection
This code casts an abstract collection (such as a Collection, List, or Set) to a specific concrete implementation (such as an ArrayList or HashSet).
This might not be correct, and it may make your code fragile, since it makes it harder to switch to other concrete implementations at a future point.
Unless you have a particular reason to do so, just use the abstract collection class.
Unchecked / unconfirmed cast
This cast is unchecked, and not all instances of the type casted from can be cast to the type it is being cast to.
Check that your program logic ensures that this cast will not fail.
Unchecked/unconfirmed cast of return value from method
This code performs an unchecked cast of the return value of a method.
The code might be calling the method in such a way that the cast is guaranteed to be safe, but FindBugs is unable to verify that the cast is safe.
Check that your program logic ensures that this cast will not fail.
instanceof will always return true
This instanceof test will always return true (unless the value being tested is null).
Although this is safe, make sure it isn't an indication of some misunderstanding or some other logic error.
If you really want to test the value for being null, perhaps it would be clearer to do better to do a null test rather than an instanceof test.
Unsigned right shift cast to short / byte
The code performs an unsigned right shift, whose result is then cast to a short or byte, which discards the upper bits of the result.
Since the upper bits are discarded, there may be no difference between a signed and unsigned right shift (depending upon the size of the shift).
Class is final but declares protected field
This class is declared to be final, but declares fields to be protected. Since the class is final, it can not be derived from, and the use of protected is confusing.
The access modifier for the field should be changed to private or public to represent the true use for the field.
Method uses the same code for two branches
This method uses the same code to implement two branches of a conditional branch.
Check to ensure that this isn't a coding mistake.
Method uses the same code for two switch clauses
This method uses the same code to implement two clauses of a switch statement.
This could be a case of duplicate code, but it might also indicate a coding mistake.
Dead store to local variable
This instruction assigns a value to a local variable, but the value is not read or used in any subsequent instruction.
Often, this indicates an error, because the value computed is never used.
Note that Sun's javac compiler often generates dead stores for final local variables.
Because FindBugs is a bytecode-based tool, there is no easy way to eliminate these false positives.
Useless assignment in return statement
This statement assigns to a local variable in a return statement.
This assignment has effect. Please verify that this statement does the right thing.
Dead store of null to local variable
The code stores null into a local variable, and the stored value is not read.
This store may have been introduced to assist the garbage collector, but as of Java SE 6.0, this is no longer needed or useful.
Dead store to local variable that shadows field
This instruction assigns a value to a local variable, but the value is not read or used in any subsequent instruction.
Often, this indicates an error, because the value computed is never used.
There is a field with the same name as the local variable. Did you mean to assign to that variable instead ?
Code contains a hard coded reference to an absolute pathname
This code constructs a File object using a hard coded to an absolute pathname
e.g., new File("/home/dannyc/workspace/j2ee/src/share/com/sun/enterprise/deployment");
Non serializable object written to ObjectOutput
This code seems to be passing a non-serializable object to the ObjectOutput.writeObject method.
If the object is, indeed, non-serializable, an error will result.
Invocation of substring(0), which returns the original value
This code invokes substring(0) on a String, which returns the original value.
Thread passed where Runnable expected
A Thread object is passed as a parameter to a method where a Runnable is expected.
This is rather unusual, and may indicate a logic error or cause unexpected behavior.
Class doesn't override equals in superclass
This class extends a class that defines an equals method and adds fields, but doesn't define an equals method itself.
Thus, equality on instances of this class will ignore the identity of the subclass and the added fields.
Be sure this is what is intended, and that you don't need to override the equals method.
Even if you don't need to override the equals method, consider overriding it anyway to document the fact that the equals method for the subclass just return the result of invoking super.equals(o).
Unusual equals method
This class doesn't do any of the patterns we recognize for checking that the type of the argument is compatible with the type of the this object.
There might not be anything wrong with this code, but it is worth reviewing.
Test for floating point equality
This operation compares two floating point values for equality.
Because floating point calculations may involve rounding, calculated float and double values may not be accurate.
For values that must be precise, such as monetary values, consider using a fixed-precision type such as BigDecimal.
For values that need not be precise, consider comparing for equality within some range,
for example
if ( Math.abs(x - y) < .0000001 )
Non-Boolean argument formatted using %b format specifier
An argument not of type Boolean is being formatted with a %b format specifier.
This won't throw an exception; instead, it will print true for any nonnull value, and false for null.
This feature of format strings is strange, and may not be what you intended.
Ambiguous invocation of either an inherited or outer method
An inner class is invoking a method that could be resolved to either a inherited method or a method defined in an outer class.
By the Java semantics, it will be resolved to invoke the inherited method, but this may not be want you intend.
If you really intend to invoke the inherited method, invoke it by invoking the method on super (e.g., invoke super.foo(17)), and thus it will be clear to other readers of your code and to FindBugs that you want to invoke the inherited method, not the method in the outer class.
Initialization circularity
A circularity was detected in the static initializers of the two classes referenced by the bug instance.
Many kinds of unexpected behavior may arise from such circularity.
integral division result cast to double or float
This code casts the result of an integral division (e.g., int or long division) operation to double or float.
Doing division on integers truncates the result to the integer value closest to zero.
The fact that the result was cast to double suggests that this precision should have been retained.
What was probably meant was to cast one or both of the operands to double before performing the division.
Here is an example :
int x = 2;
int y = 5;
// Wrong: yields result 0.0
double value1 = x / y;
// Right: yields result 0.4
double value2 = x / (double) y;
Result of integer multiplication cast to long
This code performs integer multiply and then converts the result to a long, as in:
long convertDaysToMilliseconds(int days) { return 1000*3600*24*days; }
If the multiplication is done using long arithmetic, you can avoid the possibility that the result will overflow.
For example, you could fix the above code to :
long convertDaysToMilliseconds(int days) { return 1000L*3600*24*days; }
or
static final long MILLISECONDS_PER_DAY = 24L*3600*1000;
long convertDaysToMilliseconds(int days) { return days * MILLISECONDS_PER_DAY; }
Computation of average could overflow
The code computes the average of two integers using either division or signed right shift, and then uses the result as the index of an array.
If the values being averaged are very large, this can overflow (resulting in the computation of a negative average).
Assuming that the result is intended to be nonnegative, you can use an unsigned right shift instead.
In other words, rather that using (low+high)/2, use (low+high) >>> 1
This bug exists in many earlier implementations of binary search and merge sort.
Check for oddness that won't work for negative numbers
The code uses x % 2 == 1 to check to see if a value is odd, but this won't work for negative numbers
e.g. (-5) % 2 == -1
If this code is intending to check for oddness, consider using x & 1 == 1, or x % 2 != 0
Integer remainder modulo 1
Any expression (exp % 1) is guaranteed to always return zero.
Did you mean (exp & 1) or (exp % 2) instead?
Vacuous bit mask operation on integer value
This is an integer bit operation (and, or, or exclusive or) that doesn't do any useful work (e.g., v & 0xffffffff).
Vacuous comparison of integer value
There is an integer comparison that always returns the same value (e.g., x <= Integer.MAX_VALUE).
Class extends Servlet class and uses instance variables
This class extends from a Servlet class, and uses an instance member variable.
Since only one instance of a Servlet class is created by the J2EE framework, and used in a multithreaded way, this paradigm is highly discouraged and most likely problematic.
Consider only using method local variables.
Class extends Struts Action class and uses instance variables
This class extends from a Struts Action class, and uses an instance member variable.
Since only one instance of a struts Action class is created by the Struts framework, and used in a multithreaded way, this paradigm is highly discouraged and most likely problematic.
Consider only using method local variables. Only instance fields that are written outside of a monitor are reported.
Dereference of the result of readLine() without nullcheck
The result of invoking readLine() is dereferenced without checking to see if the result is null.
If there are no more lines of text to read, readLine() will return null and dereferencing that will generate a null pointer exception.
Immediate dereference of the result of readLine()
The result of invoking readLine() is immediately dereferenced.
If there are no more lines of text to read, readLine() will return null and dereferencing that will generate a null pointer exception.
Load of known null value
The variable referenced at this point is known to be null due to an earlier check against null.
Although this is valid, it might be a mistake (perhaps you intended to refer to a different variable, or perhaps the earlier check to see if the variable is null should have been a check to see if it was nonnull).
Possible null pointer dereference due to return value of called method
The return value from a method is dereferenced without a null check, and the return value of that method is one that should generally be checked for null.
This may lead to a NullPointerException when the code is executed.
Possible null pointer dereference on branch that might be infeasible
There is a branch of statement that, if executed, guarantees that a null value will be dereferenced, which would generate a NullPointerException when the code is executed.
Of course, the problem might be that the branch or statement is infeasible and that the null pointer exception can't ever be executed; deciding that is beyond the ability of FindBugs.
Due to the fact that this value had been previously tested for nullness, this is a definite possibility.
Parameter must be nonnull but is marked as nullable
This parameter is always used in a way that requires it to be nonnull, but the parameter is explicitly annotated as being Nullable.
Either the use of the parameter or the annotation is wrong.
Read of unwritten public or protected field
The program is dereferencing a public or protected field that does not seem to ever have a non-null value written to it.
Unless the field is initialized via some mechanism not seen by the analysis, dereferencing this value will generate a null pointer exception.
Potentially dangerous use of non-short-circuit logic
This code seems to be using non-short-circuit logic (e.g., & or |) rather than short-circuit logic (&& or ||).
In addition, it seem possible that, depending on the value of the left hand side, you might not want to evaluate the right hand side (because it would have side effects, could cause an exception or could be expensive.
Non-short-circuit logic causes both sides of the expression to be evaluated even when the result can be inferred from knowing the left-hand side.
This can be less efficient and can result in errors if the left-hand side guards cases when evaluating the right-hand side can generate an error.
Questionable use of non-short-circuit logic
This code seems to be using non-short-circuit logic (e.g., & or |) rather than short-circuit logic (&& or ||).
Non-short-circuit logic causes both sides of the expression to be evaluated even when the result can be inferred from knowing the left-hand side.
This can be less efficient and can result in errors if the left-hand side guards cases when evaluating the right-hand side can generate an error.
Consider returning a zero length array rather than null
It is often a better design to return a length zero array rather than a null reference to indicate that there are no results (i.e., an empty list of results).
This way, no explicit check for null is needed by clients of the method.
On the other hand, using null to indicate "there is no answer to this question" is probably appropriate.
For example, File.listFiles() returns an empty list if given a directory containing no files, and returns null if the file is not a directory.
Complicated, subtle or wrong increment in for-loop
Are you sure this for loop is incrementing the correct variable?
It appears that another variable is being initialized and checked by the for loop.
Redundant comparison of non-null value to null
This method contains a reference known to be non-null with another reference known to be null.
Redundant comparison of two null values
This method contains a redundant comparison of two references known to both be definitely null.
Redundant nullcheck of value known to be non-null
This method contains a redundant check of a known non-null value against the constant null.
Redundant nullcheck of value known to be null
This method contains a redundant check of a known null value against the constant null.
Exception is caught when Exception is not thrown
This method uses a try-catch block that catches Exception objects, but Exception is not thrown within the try block, and RuntimeException is not explicitly caught.
It is a common bug pattern to say try { ... } catch (Exception e) { something } as a shorthand for catching a number of types of exception each of whose catch blocks is identical, but this construct also accidentally catches RuntimeException as well, masking potential bugs.
A better approach is to either explicitly catch the specific exceptions that are thrown, or to explicitly catch RuntimeException exception, rethrow it, and then catch all non-Runtime Exceptions, as shown below :
try {
...
} catch (RuntimeException e) {
throw e;
} catch (Exception e) {
... deal with all non-runtime exceptions ...
}
Class implements same interface as superclass
This class declares that it implements an interface that is also implemented by a superclass.
This is redundant because once a superclass implements an interface, all subclasses by default also implement this interface.
It may point out that the inheritance hierarchy has changed since this class was created, and consideration should be given to the ownership of the interface's implementation.
Method checks to see if result of String.indexOf is positive
The method invokes String.indexOf and checks to see if the result is positive or non-positive.
It is much more typical to check to see if the result is negative or non-negative.
It is positive only if the substring checked for occurs at some place other than at the beginning of the String.
Method discards result of readLine after checking if it is nonnull
The value returned by readLine is discarded after checking to see if the return value is non-null.
In almost all situations, if the result is non-null, you will want to use that non-null value.
Calling readLine again will give you a different line.
Remainder of hashCode could be negative
This code computes a hashCode, and then computes the remainder of that value modulo another value.
Since the hashCode can be negative, the result of the remainder operation can also be negative.
Assuming you want to ensure that the result of your computation is nonnegative, you may need to change your code.
If you know the divisor is a power of 2, you can use a bitwise and operator instead
i.e., instead of using x.hashCode()%n, use x.hashCode()&(n-1)
This is probably faster than computing the remainder as well.
If you don't know that the divisor is a power of 2, take the absolute value of the result of the remainder operation
i.e., use Math.abs(x.hashCode()%n
Remainder of 32-bit signed random integer
This code generates a random signed integer and then computes the remainder of that value modulo another value.
Since the random number can be negative, the result of the remainder operation can also be negative.
Be sure this is intended, and strongly consider using the Random.nextInt(int) method instead.
Method ignores return value, is this OK ?
This code calls a method and ignores the return value.
The return value is the same type as the type the method is invoked on, and from our analysis it looks like the return value might be important e.g., like ignoring the return value of String.toLowerCase()
We are guessing that ignoring the return value might be a bad idea just from a simple analysis of the body of the method.
You can use a @CheckReturnValue annotation to instruct FindBugs as to whether ignoring the return value of this method is important or acceptable.
Please investigate this closely to decide whether it is OK to ignore the return value.
Double assignment of field
This method contains a double assignment of a field; e.g.
int x,y;
public void foo() {
x = x = 17;
}
Assigning to a field twice is useless, and may indicate a logic error or typo.
Double assignment of local variable
This method contains a double assignment of a local variable; e.g.
public void foo() {
int x,y;
x = x = 17;
}
Assigning the same value to a variable twice is useless, and may indicate a logic error or typo.
Self assignment of local variable
This method contains a self assignment of a local variable; e.g.
public void foo() {
int x = 3;
x = x;
}
Such assignments are useless, and may indicate a logic error or typo.
Switch statement found where one case falls through to the next case
This method contains a switch statement where one case branch will fall through to the next case.
Usually you need to end this case with a break or return.
Switch statement found where default case is missing
This method contains a switch statement where default case is missing. Usually you need to provide a default case.
Because the analysis only looks at the generated bytecode, this warning can be incorrect triggered if the default case is at the end of the switch statement and doesn't end with a break statement.
Write to static field from instance method
This instance method writes to a static field.
This is tricky to get correct if multiple instances are being manipulated, and generally bad practice.
private readResolve method not inherited by subclasses
This class defines a private readResolve method.
Since it is private, it won't be inherited by subclasses.
This might be intentional and OK, but should be reviewed to ensure it is what is intended.
Transient field of class that isn't Serializable.
The field is marked as transient, but the class isn't Serializable, so marking it as transient has absolutely no effect.
This may be leftover marking from a previous version of the code in which the class was transient, or it may indicate a misunderstanding of how serialization works.
Value required to have type qualifier, but marked as unknown
A value is used in a way that requires it to be always be a value denoted by a type qualifier, but there is an explicit annotation stating that it is not known where the value is required to have that type qualifier.
Either the usage or the annotation is incorrect.
Value required to not have type qualifier, but marked as unknown
A value is used in a way that requires it to be never be a value denoted by a type qualifier, but there is an explicit annotation stating that it is not known where the value is prohibited from having that type qualifier.
Either the usage or the annotation is incorrect.
Useless control flow
This method contains a useless control flow statement, where control flow continues onto the same place regardless of whether or not the branch is taken.
For example, this is caused by having an empty statement block for an if statement:
if (argv.length == 0) {
// TODO: handle this case
}
Useless control flow to next line
This method contains a useless control flow statement in which control flow follows to the same or following line regardless of whether or not the branch is taken.
Often, this is caused by inadvertently using an empty statement as the body of an if statement, e.g.:
if (argv.length == 1);
System.out.println("Hello, " + argv[0]);
Unread public / protected field
This field is never read.
The field is public or protected, so perhaps it is intended to be used with classes not seen as part of the analysis.
If not, consider removing it from the class.
Unused public or protected field
This field is never used.
The field is public or protected, so perhaps it is intended to be used with classes not seen as part of the analysis.
If not, consider removing it from the class.
Field not initialized in constructor but dereferenced without null check
This field is never initialized within any constructor, and is therefore could be null after the object is constructed.
Elsewhere, it is loaded and dereferenced without a null check.
This could be a either an error or a questionable design, since it means a null pointer exception will be generated if that field is dereferenced before being initialized.
Unwritten public or protected field
No writes were seen to this public/protected field.
All reads of it will return the default value.
Check for errors (should it have been initialized?), or remove it if it is useless.
Method directly allocates a specific implementation of xml interfaces
This method allocates a specific implementation of an xml interface.
It is preferable to use the supplied factory classes to create these objects so that the implementation can be changed at runtime.
See
javax.xml.parsers.DocumentBuilderFactory
javax.xml.parsers.SAXParserFactory
javax.xml.transform.TransformerFactory
org.w3c.dom.Document.createXXXX
for details.
No comments:
Post a Comment
Note: only a member of this blog may post a comment.