Showing 518 of 532 total issues
Define a constant instead of duplicating this literal "CreateShortcuts" 4 times. Open
LogHelper.i("CreateShortcuts", "Attempting to create shortcut for " + className);
- Read upRead up
- Exclude checks
Duplicated string literals make the process of refactoring error-prone, since you must be sure to update all occurrences.
On the other hand, constants can be referenced from many places, but only need to be updated in a single place.
Noncompliant Code Example
With the default threshold of 3:
public void run() { prepare("action1"); // Noncompliant - "action1" is duplicated 3 times execute("action1"); release("action1"); } @SuppressWarning("all") // Compliant - annotations are excluded private void method1() { /* ... */ } @SuppressWarning("all") private void method2() { /* ... */ } public String method3(String a) { System.out.println("'" + a + "'"); // Compliant - literal "'" has less than 5 characters and is excluded return ""; // Compliant - literal "" has less than 5 characters and is excluded }
Compliant Solution
private static final String ACTION_1 = "action1"; // Compliant public void run() { prepare(ACTION_1); // Compliant execute(ACTION_1); release(ACTION_1); }
Exceptions
To prevent generating some false-positives, literals having less than 5 characters are excluded.
Make the enclosing method "static" or remove this set. Open
err_gpu_clock_count++;
- Read upRead up
- Exclude checks
Correctly updating a static
field from a non-static method is tricky to get right and could easily lead to bugs if there are multiple
class instances and/or multiple threads in play. Ideally, static
fields are only updated from synchronized static
methods.
This rule raises an issue each time a static
field is updated from a non-static method.
Noncompliant Code Example
public class MyClass { private static int count = 0; public void doSomething() { //... count++; // Noncompliant } }
Use try-with-resources or close this "RandomAccessFile" in a "finally" clause. Open
RandomAccessFile file = new RandomAccessFile("/proc/mounts", "r");
- Read upRead up
- Exclude checks
Connections, streams, files, and other classes that implement the Closeable
interface or its super-interface,
AutoCloseable
, needs to be closed after use. Further, that close
call must be made in a finally
block otherwise
an exception could keep the call from being made. Preferably, when class implements AutoCloseable
, resource should be created using
"try-with-resources" pattern and will be closed automatically.
Failure to properly close resources will result in a resource leak which could bring first the application and then perhaps the box the application is on to their knees.
Noncompliant Code Example
private void readTheFile() throws IOException { Path path = Paths.get(this.fileName); BufferedReader reader = Files.newBufferedReader(path, this.charset); // ... reader.close(); // Noncompliant // ... Files.lines("input.txt").forEach(System.out::println); // Noncompliant: The stream needs to be closed } private void doSomething() { OutputStream stream = null; try { for (String property : propertyList) { stream = new FileOutputStream("myfile.txt"); // Noncompliant // ... } } catch (Exception e) { // ... } finally { stream.close(); // Multiple streams were opened. Only the last is closed. } }
Compliant Solution
private void readTheFile(String fileName) throws IOException { Path path = Paths.get(fileName); try (BufferedReader reader = Files.newBufferedReader(path, StandardCharsets.UTF_8)) { reader.readLine(); // ... } // .. try (Stream<String> input = Files.lines("input.txt")) { input.forEach(System.out::println); } } private void doSomething() { OutputStream stream = null; try { stream = new FileOutputStream("myfile.txt"); for (String property : propertyList) { // ... } } catch (Exception e) { // ... } finally { stream.close(); } }
Exceptions
Instances of the following classes are ignored by this rule because close
has no effect:
-
java.io.ByteArrayOutputStream
-
java.io.ByteArrayInputStream
-
java.io.CharArrayReader
-
java.io.CharArrayWriter
-
java.io.StringReader
-
java.io.StringWriter
Java 7 introduced the try-with-resources statement, which implicitly closes Closeables
. All resources opened in a try-with-resources
statement are ignored by this rule.
try (BufferedReader br = new BufferedReader(new FileReader(fileName))) { //... } catch ( ... ) { //... }
See
- MITRE, CWE-459 - Incomplete Cleanup
- MITRE, CWE-772 - Missing Release of Resource after Effective Lifetime
- CERT, FIO04-J. - Release resources when they are no longer needed
- CERT, FIO42-C. - Close files when they are no longer needed
- Try With Resources
This block of commented-out lines of code should be removed. Open
// Licensed under the Apache License, Version 2.0 (the "License");
- Read upRead up
- Exclude checks
Programmers should not comment out code as it bloats programs and reduces readability.
Unused code should be deleted and can be retrieved from source control history if required.
Either re-interrupt this method or rethrow the "InterruptedException" that can be caught here. Open
} catch (InterruptedException e) {
- Read upRead up
- Exclude checks
InterruptedExceptions
should never be ignored in the code, and simply logging the exception counts in this case as "ignoring". The
throwing of the InterruptedException
clears the interrupted state of the Thread, so if the exception is not handled properly the fact
that the thread was interrupted will be lost. Instead, InterruptedExceptions
should either be rethrown - immediately or after cleaning up
the method's state - or the thread should be re-interrupted by calling Thread.interrupt()
even if this is supposed to be a
single-threaded application. Any other course of action risks delaying thread shutdown and loses the information that the thread was interrupted -
probably without finishing its task.
Similarly, the ThreadDeath
exception should also be propagated. According to its JavaDoc:
If
ThreadDeath
is caught by a method, it is important that it be rethrown so that the thread actually dies.
Noncompliant Code Example
public void run () { try { while (true) { // do stuff } }catch (InterruptedException e) { // Noncompliant; logging is not enough LOGGER.log(Level.WARN, "Interrupted!", e); } }
Compliant Solution
public void run () { try { while (true) { // do stuff } }catch (InterruptedException e) { LOGGER.log(Level.WARN, "Interrupted!", e); // Restore interrupted state... Thread.currentThread().interrupt(); } }
See
- MITRE, CWE-391 - Unchecked Error Condition
- Dealing with InterruptedException
Make the enclosing method "static" or remove this set. Open
if (nowPlaying == null) nowPlaying = new NowPlaying();
- Read upRead up
- Exclude checks
Correctly updating a static
field from a non-static method is tricky to get right and could easily lead to bugs if there are multiple
class instances and/or multiple threads in play. Ideally, static
fields are only updated from synchronized static
methods.
This rule raises an issue each time a static
field is updated from a non-static method.
Noncompliant Code Example
public class MyClass { private static int count = 0; public void doSomething() { //... count++; // Noncompliant } }
Return an empty array instead of null. Open
return null;
- Read upRead up
- Exclude checks
Returning null
instead of an actual array or collection forces callers of the method to explicitly test for nullity, making them more
complex and less readable.
Moreover, in many cases, null
is used as a synonym for empty.
Noncompliant Code Example
public static List<Result> getResults() { return null; // Noncompliant } public static Result[] getResults() { return null; // Noncompliant } public static void main(String[] args) { Result[] results = getResults(); if (results != null) { // Nullity test required to prevent NPE for (Result result: results) { /* ... */ } } }
Compliant Solution
public static List<Result> getResults() { return Collections.emptyList(); // Compliant } public static Result[] getResults() { return new Result[0]; } public static void main(String[] args) { for (Result result: getResults()) { /* ... */ } }
See
- CERT, MSC19-C. - For functions that return an array, prefer returning an empty array over a null value
- CERT, MET55-J. - Return an empty array or collection instead of a null value for methods that return an array or collection
Use try-with-resources or close this "FileOutputStream" in a "finally" clause. Open
OutputStream out = new FileOutputStream(outFile);
- Read upRead up
- Exclude checks
Connections, streams, files, and other classes that implement the Closeable
interface or its super-interface,
AutoCloseable
, needs to be closed after use. Further, that close
call must be made in a finally
block otherwise
an exception could keep the call from being made. Preferably, when class implements AutoCloseable
, resource should be created using
"try-with-resources" pattern and will be closed automatically.
Failure to properly close resources will result in a resource leak which could bring first the application and then perhaps the box the application is on to their knees.
Noncompliant Code Example
private void readTheFile() throws IOException { Path path = Paths.get(this.fileName); BufferedReader reader = Files.newBufferedReader(path, this.charset); // ... reader.close(); // Noncompliant // ... Files.lines("input.txt").forEach(System.out::println); // Noncompliant: The stream needs to be closed } private void doSomething() { OutputStream stream = null; try { for (String property : propertyList) { stream = new FileOutputStream("myfile.txt"); // Noncompliant // ... } } catch (Exception e) { // ... } finally { stream.close(); // Multiple streams were opened. Only the last is closed. } }
Compliant Solution
private void readTheFile(String fileName) throws IOException { Path path = Paths.get(fileName); try (BufferedReader reader = Files.newBufferedReader(path, StandardCharsets.UTF_8)) { reader.readLine(); // ... } // .. try (Stream<String> input = Files.lines("input.txt")) { input.forEach(System.out::println); } } private void doSomething() { OutputStream stream = null; try { stream = new FileOutputStream("myfile.txt"); for (String property : propertyList) { // ... } } catch (Exception e) { // ... } finally { stream.close(); } }
Exceptions
Instances of the following classes are ignored by this rule because close
has no effect:
-
java.io.ByteArrayOutputStream
-
java.io.ByteArrayInputStream
-
java.io.CharArrayReader
-
java.io.CharArrayWriter
-
java.io.StringReader
-
java.io.StringWriter
Java 7 introduced the try-with-resources statement, which implicitly closes Closeables
. All resources opened in a try-with-resources
statement are ignored by this rule.
try (BufferedReader br = new BufferedReader(new FileReader(fileName))) { //... } catch ( ... ) { //... }
See
- MITRE, CWE-459 - Incomplete Cleanup
- MITRE, CWE-772 - Missing Release of Resource after Effective Lifetime
- CERT, FIO04-J. - Release resources when they are no longer needed
- CERT, FIO42-C. - Close files when they are no longer needed
- Try With Resources
Make the enclosing method "static" or remove this set. Open
processing = false;
- Read upRead up
- Exclude checks
Correctly updating a static
field from a non-static method is tricky to get right and could easily lead to bugs if there are multiple
class instances and/or multiple threads in play. Ideally, static
fields are only updated from synchronized static
methods.
This rule raises an issue each time a static
field is updated from a non-static method.
Noncompliant Code Example
public class MyClass { private static int count = 0; public void doSomething() { //... count++; // Noncompliant } }
Catch Exception instead of Throwable. Open
} catch (Throwable t) {
- Read upRead up
- Exclude checks
Throwable
is the superclass of all errors and exceptions in Java. Error
is the superclass of all errors, which are not
meant to be caught by applications.
Catching either Throwable
or Error
will also catch OutOfMemoryError
and InternalError
, from
which an application should not attempt to recover.
Noncompliant Code Example
try { /* ... */ } catch (Throwable t) { /* ... */ } try { /* ... */ } catch (Error e) { /* ... */ }
Compliant Solution
try { /* ... */ } catch (RuntimeException e) { /* ... */ } try { /* ... */ } catch (MyException e) { /* ... */ }
See
- MITRE, CWE-396 - Declaration of Catch for Generic Exception
- C++ Core Guidelines E.14 - Use purpose-designed user-defined types as exceptions (not built-in types)
Refactor this method to reduce its Cognitive Complexity from 20 to the 15 allowed. Open
private void scanGhostDir() {
- Read upRead up
- Exclude checks
Cognitive Complexity is a measure of how hard the control flow of a method is to understand. Methods with high Cognitive Complexity will be difficult to maintain.
See
Add a private constructor to hide the implicit public one. Open
class Utils {
- Read upRead up
- Exclude checks
Utility classes, which are collections of static
members, are not meant to be instantiated. Even abstract utility classes, which can
be extended, should not have public constructors.
Java adds an implicit public constructor to every class which does not define at least one explicitly. Hence, at least one non-public constructor should be defined.
Noncompliant Code Example
class StringUtils { // Noncompliant public static String concatenate(String s1, String s2) { return s1 + s2; } }
Compliant Solution
class StringUtils { // Compliant private StringUtils() { throw new IllegalStateException("Utility class"); } public static String concatenate(String s1, String s2) { return s1 + s2; } }
Exceptions
When class contains public static void main(String[] args)
method it is not considered as utility class and will be ignored by this
rule.
This block of commented-out lines of code should be removed. Open
// Licensed under the Apache License, Version 2.0 (the "License");
- Read upRead up
- Exclude checks
Programmers should not comment out code as it bloats programs and reduces readability.
Unused code should be deleted and can be retrieved from source control history if required.
Refactor this method to reduce its Cognitive Complexity from 24 to the 15 allowed. Open
public void onAccessibilityEvent(AccessibilityEvent event) {
- Read upRead up
- Exclude checks
Cognitive Complexity is a measure of how hard the control flow of a method is to understand. Methods with high Cognitive Complexity will be difficult to maintain.
See
Refactor the code in order to not assign to this loop counter from within the loop body. Open
i += 3;
- Read upRead up
- Exclude checks
A for
loop stop condition should test the loop counter against an invariant value (i.e. one that is true at both the beginning and
ending of every loop iteration). Ideally, this means that the stop condition is set to a local variable just before the loop begins.
Stop conditions that are not invariant are slightly less efficient, as well as being difficult to understand and maintain, and likely lead to the introduction of errors in the future.
This rule tracks three types of non-invariant stop conditions:
- When the loop counters are updated in the body of the
for
loop - When the stop condition depend upon a method call
- When the stop condition depends on an object property, since such properties could change during the execution of the loop.
Noncompliant Code Example
for (int i = 0; i < 10; i++) { ... i = i - 1; // Noncompliant; counter updated in the body of the loop ... }
Compliant Solution
for (int i = 0; i < 10; i++) {...}
Add a default case to this switch. Open
switch ((int) pdoption) {
- Read upRead up
- Exclude checks
The requirement for a final default
clause is defensive programming. The clause should either take appropriate action, or contain a
suitable comment as to why no action is taken.
Noncompliant Code Example
switch (param) { //missing default clause case 0: doSomething(); break; case 1: doSomethingElse(); break; } switch (param) { default: // default clause should be the last one error(); break; case 0: doSomething(); break; case 1: doSomethingElse(); break; }
Compliant Solution
switch (param) { case 0: doSomething(); break; case 1: doSomethingElse(); break; default: error(); break; }
Exceptions
If the switch
parameter is an Enum
and if all the constants of this enum are used in the case
statements,
then no default
clause is expected.
Example:
public enum Day { SUNDAY, MONDAY } ... switch(day) { case SUNDAY: doSomething(); break; case MONDAY: doSomethingElse(); break; }
See
- MITRE, CWE-478 - Missing Default Case in Switch Statement
- CERT, MSC01-C. - Strive for logical completeness
Refactor the code in order to not assign to this loop counter from within the loop body. Open
i = thetext.length();
- Read upRead up
- Exclude checks
A for
loop stop condition should test the loop counter against an invariant value (i.e. one that is true at both the beginning and
ending of every loop iteration). Ideally, this means that the stop condition is set to a local variable just before the loop begins.
Stop conditions that are not invariant are slightly less efficient, as well as being difficult to understand and maintain, and likely lead to the introduction of errors in the future.
This rule tracks three types of non-invariant stop conditions:
- When the loop counters are updated in the body of the
for
loop - When the stop condition depend upon a method call
- When the stop condition depends on an object property, since such properties could change during the execution of the loop.
Noncompliant Code Example
for (int i = 0; i < 10; i++) { ... i = i - 1; // Noncompliant; counter updated in the body of the loop ... }
Compliant Solution
for (int i = 0; i < 10; i++) {...}
Refactor this method to reduce its Cognitive Complexity from 21 to the 15 allowed. Open
private static void firstMove(String[][] gameBoard, int lastRow, int lastCol, Context context)
- Read upRead up
- Exclude checks
Cognitive Complexity is a measure of how hard the control flow of a method is to understand. Methods with high Cognitive Complexity will be difficult to maintain.
See
Add a private constructor to hide the implicit public one. Open
public class JsonHelper {
- Read upRead up
- Exclude checks
Utility classes, which are collections of static
members, are not meant to be instantiated. Even abstract utility classes, which can
be extended, should not have public constructors.
Java adds an implicit public constructor to every class which does not define at least one explicitly. Hence, at least one non-public constructor should be defined.
Noncompliant Code Example
class StringUtils { // Noncompliant public static String concatenate(String s1, String s2) { return s1 + s2; } }
Compliant Solution
class StringUtils { // Compliant private StringUtils() { throw new IllegalStateException("Utility class"); } public static String concatenate(String s1, String s2) { return s1 + s2; } }
Exceptions
When class contains public static void main(String[] args)
method it is not considered as utility class and will be ignored by this
rule.
A "NullPointerException" could be thrown; "receivers" is nullable here. Open
if (!receiverList.isEmpty()) creator.addView(generateSingleColumn("Receivers (" + receivers.length + ")", receiverList));
- Read upRead up
- Exclude checks
A reference to null
should never be dereferenced/accessed. Doing so will cause a NullPointerException
to be thrown. At
best, such an exception will cause abrupt program termination. At worst, it could expose debugging information that would be useful to an attacker, or
it could allow an attacker to bypass security measures.
Note that when they are present, this rule takes advantage of @CheckForNull
and @Nonnull
annotations defined in JSR-305 to understand which values are and are not nullable except when @Nonnull
is used
on the parameter to equals
, which by contract should always work with null.
Noncompliant Code Example
@CheckForNull String getName(){...} public boolean isNameEmpty() { return getName().length() == 0; // Noncompliant; the result of getName() could be null, but isn't null-checked }
Connection conn = null; Statement stmt = null; try{ conn = DriverManager.getConnection(DB_URL,USER,PASS); stmt = conn.createStatement(); // ... }catch(Exception e){ e.printStackTrace(); }finally{ stmt.close(); // Noncompliant; stmt could be null if an exception was thrown in the try{} block conn.close(); // Noncompliant; conn could be null if an exception was thrown }
private void merge(@Nonnull Color firstColor, @Nonnull Color secondColor){...} public void append(@CheckForNull Color color) { merge(currentColor, color); // Noncompliant; color should be null-checked because merge(...) doesn't accept nullable parameters }
void paint(Color color) { if(color == null) { System.out.println("Unable to apply color " + color.toString()); // Noncompliant; NullPointerException will be thrown return; } ... }
See
- MITRE, CWE-476 - NULL Pointer Dereference
- CERT, EXP34-C. - Do not dereference null pointers
- CERT, EXP01-J. - Do not use a null in a case where an object is required