Showing 716 of 716 total issues
Remove this unused "almServerSettingsModel" private field. Open
private AlmServerSettingsModel almServerSettingsModel;
- Read upRead up
- Exclude checks
If a private
field is declared but not used in the program, it can be considered dead code and should therefore be removed. This will
improve maintainability because developers will not wonder what the variable is used for.
Note that this rule does not take reflection into account, which means that issues will be raised on private
fields that are only
accessed using the reflection API.
Noncompliant Code Example
public class MyClass { private int foo = 42; public int compute(int a) { return a * 42; } }
Compliant Solution
public class MyClass { public int compute(int a) { return a * 42; } }
Exceptions
The Java serialization runtime associates with each serializable class a version number, called serialVersionUID
, which is used during
deserialization to verify that the sender and receiver of a serialized object have loaded classes for that object that are compatible with respect to
serialization.
A serializable class can declare its own serialVersionUID
explicitly by declaring a field named serialVersionUID
that
must be static, final, and of type long. By definition those serialVersionUID
fields should not be reported by this rule:
public class MyClass implements java.io.Serializable { private static final long serialVersionUID = 42L; }
Moreover, this rule doesn't raise any issue on annotated fields.
Use already-defined constant 'COLLATE_ANALYZE' instead of duplicating its value here. Open
"CollateAndAnalyze");
- 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.
Rename this constant name to match the regular expression '^[A-Z][A-Z0-9]*(_[A-Z0-9]+)*$'. Open
private static final String OsTypeKey = "ostype";
- Read upRead up
- Exclude checks
Shared coding conventions allow teams to collaborate efficiently. This rule checks that all constant names match a provided regular expression.
Noncompliant Code Example
With the default regular expression ^[A-Z][A-Z0-9]*(_[A-Z0-9]+)*$
:
public class MyClass { public static final int first = 1; } public enum MyEnum { first; }
Compliant Solution
public class MyClass { public static final int FIRST = 1; } public enum MyEnum { FIRST; }
Constructor has 9 parameters, which is greater than 7 authorized. Open
public SseBuildAndPublishStep(String almServerName,
- Read upRead up
- Exclude checks
A long parameter list can indicate that a new structure should be created to wrap the numerous parameters or that the function is doing too many things.
Noncompliant Code Example
With a maximum number of 4 parameters:
public void doSomething(int param1, int param2, int param3, String param4, long param5) { ... }
Compliant Solution
public void doSomething(int param1, int param2, int param3, String param4) { ... }
Exceptions
Methods annotated with :
- Spring's
@RequestMapping
(and related shortcut annotations, like@GetRequest
) - JAX-RS API annotations (like
@javax.ws.rs.GET
) - Bean constructor injection with
@org.springframework.beans.factory.annotation.Autowired
- CDI constructor injection with
@javax.inject.Inject
-
@com.fasterxml.jackson.annotation.JsonCreator
may have a lot of parameters, encapsulation being possible. Such methods are therefore ignored.
Refactor this method to reduce its Cognitive Complexity from 16 to the 15 allowed. Open
public static void runOnBuildEnv(
- 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
A "NullPointerException" could be thrown; "build" is nullable here. Open
build.setResult(Result.ABORTED);
- 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
"NullPointerException" will be thrown when invoking method "parsePcRunResponse()". Open
parsePcRunResponse(ret, response, build, errorMessage, eventLogString);
- 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
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
Either re-interrupt this method or rethrow the "InterruptedException" that can be caught here. Open
} catch (IOException | 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
Refactor this method to not always return the same value. Open
private boolean saveFileToWorkspacePath(PcClient pcClient, String trendReportID, int runId, TrendReportTypes.DataType dataType, TrendReportTypes.PctType pctType, TrendReportTypes.Measurement measurement) throws IOException, PcException, IntrospectionException, NoSuchMethodException {
- Read upRead up
- Exclude checks
When a method is designed to return an invariant value, it may be poor design, but it shouldn't adversely affect the outcome of your program. However, when it happens on all paths through the logic, it is surely a bug.
This rule raises an issue when a method contains several return
statements that all return the same value.
Noncompliant Code Example
int foo(int a) { int b = 12; if (a == 1) { return b; } return b; // Noncompliant }
Rename this constant name to match the regular expression '^[A-Z][A-Z0-9]*(_[A-Z0-9]+)*$'. Open
public static final String trendReportStructure = "%s/%s/performanceTestsReports/TrendReports";
- Read upRead up
- Exclude checks
Shared coding conventions allow teams to collaborate efficiently. This rule checks that all constant names match a provided regular expression.
Noncompliant Code Example
With the default regular expression ^[A-Z][A-Z0-9]*(_[A-Z0-9]+)*$
:
public class MyClass { public static final int first = 1; } public enum MyEnum { first; }
Compliant Solution
public class MyClass { public static final int FIRST = 1; } public enum MyEnum { FIRST; }
Rename this constant name to match the regular expression '^[A-Z][A-Z0-9]*(_[A-Z0-9]+)*$'. Open
public static final String pcReportFileName = "Report.html";
- Read upRead up
- Exclude checks
Shared coding conventions allow teams to collaborate efficiently. This rule checks that all constant names match a provided regular expression.
Noncompliant Code Example
With the default regular expression ^[A-Z][A-Z0-9]*(_[A-Z0-9]+)*$
:
public class MyClass { public static final int first = 1; } public enum MyEnum { first; }
Compliant Solution
public class MyClass { public static final int FIRST = 1; } public enum MyEnum { FIRST; }
Extract this nested ternary operation into an independent statement. Open
this.retryOccurrences = ("NO_RETRY".equals(this.retry)) ? "0" : (retryOccurrences == null || retryOccurrences.isEmpty()) ? "3" : retryOccurrences;
- Read upRead up
- Exclude checks
Just because you can do something, doesn't mean you should, and that's the case with nested ternary operations. Nesting ternary operators results in the kind of code that may seem clear as day when you write it, but six months later will leave maintainers (or worse - future you) scratching their heads and cursing.
Instead, err on the side of clarity, and use another line to express the nested operation as a separate statement.
Noncompliant Code Example
public String getReadableStatus(Job j) { return j.isRunning() ? "Running" : j.hasErrors() ? "Failed" : "Succeeded"; // Noncompliant }
Compliant Solution
public String getReadableStatus(Job j) { if (j.isRunning()) { return "Running"; } return j.hasErrors() ? "Failed" : "Succeeded"; }
This block of commented-out lines of code should be removed. Open
// public static FormValidation CheckOnlyAutoTestInstanceId(String autoTestInstanceID){
- 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.
Replace this alternation with a character class. Open
if (!StringUtils.isEmpty(ignoreErrorStrings.replaceAll("\\r|\\n", ""))){
- Read upRead up
- Exclude checks
When an alternation contains multiple alternatives that consist of a single character, it can be rewritten as a character class. This should be preferred because it is more efficient and can even help prevent stack overflows when used inside a repetition (see rule {rule:java:S5998} ).
Noncompliant Code Example
Pattern.compile("a|b|c"); // Noncompliant
Compliant Solution
Pattern.compile("[abc]"); // or Pattern.compile("[a-c]");
Constructor has 15 parameters, which is greater than 7 authorized. Open
public AUTEnvironmentResolvedModel(
- Read upRead up
- Exclude checks
A long parameter list can indicate that a new structure should be created to wrap the numerous parameters or that the function is doing too many things.
Noncompliant Code Example
With a maximum number of 4 parameters:
public void doSomething(int param1, int param2, int param3, String param4, long param5) { ... }
Compliant Solution
public void doSomething(int param1, int param2, int param3, String param4) { ... }
Exceptions
Methods annotated with :
- Spring's
@RequestMapping
(and related shortcut annotations, like@GetRequest
) - JAX-RS API annotations (like
@javax.ws.rs.GET
) - Bean constructor injection with
@org.springframework.beans.factory.annotation.Autowired
- CDI constructor injection with
@javax.inject.Inject
-
@com.fasterxml.jackson.annotation.JsonCreator
may have a lot of parameters, encapsulation being possible. Such methods are therefore ignored.
Remove this unused "logger" private field. Open
private static final Logger logger = Logger
- Read upRead up
- Exclude checks
If a private
field is declared but not used in the program, it can be considered dead code and should therefore be removed. This will
improve maintainability because developers will not wonder what the variable is used for.
Note that this rule does not take reflection into account, which means that issues will be raised on private
fields that are only
accessed using the reflection API.
Noncompliant Code Example
public class MyClass { private int foo = 42; public int compute(int a) { return a * 42; } }
Compliant Solution
public class MyClass { public int compute(int a) { return a * 42; } }
Exceptions
The Java serialization runtime associates with each serializable class a version number, called serialVersionUID
, which is used during
deserialization to verify that the sender and receiver of a serialized object have loaded classes for that object that are compatible with respect to
serialization.
A serializable class can declare its own serialVersionUID
explicitly by declaring a field named serialVersionUID
that
must be static, final, and of type long. By definition those serialVersionUID
fields should not be reported by this rule:
public class MyClass implements java.io.Serializable { private static final long serialVersionUID = 42L; }
Moreover, this rule doesn't raise any issue on annotated fields.
A "NullPointerException" could be thrown; "credentials" is nullable here. Open
credentials.getUsername(),
- 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
Method has 11 parameters, which is greater than 7 authorized. Open
public Map<String, String> getJobId(String mcUrl, String mcUserName, String mcPassword, String mcTenantId, String accessKey, String authType,
- Read upRead up
- Exclude checks
A long parameter list can indicate that a new structure should be created to wrap the numerous parameters or that the function is doing too many things.
Noncompliant Code Example
With a maximum number of 4 parameters:
public void doSomething(int param1, int param2, int param3, String param4, long param5) { ... }
Compliant Solution
public void doSomething(int param1, int param2, int param3, String param4) { ... }
Exceptions
Methods annotated with :
- Spring's
@RequestMapping
(and related shortcut annotations, like@GetRequest
) - JAX-RS API annotations (like
@javax.ws.rs.GET
) - Bean constructor injection with
@org.springframework.beans.factory.annotation.Autowired
- CDI constructor injection with
@javax.inject.Inject
-
@com.fasterxml.jackson.annotation.JsonCreator
may have a lot of parameters, encapsulation being possible. Such methods are therefore ignored.
Remove this useless assignment to local variable "varResolver". Open
VariableResolver<String> varResolver = ((AbstractBuild) build).getBuildVariableResolver();
- Read upRead up
- Exclude checks
A dead store happens when a local variable is assigned a value that is not read by any subsequent instruction. Calculating or retrieving a value only to then overwrite it or throw it away, could indicate a serious error in the code. Even if it's not an error, it is at best a waste of resources. Therefore all calculated values should be used.
Noncompliant Code Example
i = a + b; // Noncompliant; calculation result not used before value is overwritten i = compute();
Compliant Solution
i = a + b; i += compute();
Exceptions
This rule ignores initializations to -1, 0, 1, null
, true
, false
and ""
.
See
- MITRE, CWE-563 - Assignment to Variable without Use ('Unused Variable')
- CERT, MSC13-C. - Detect and remove unused values
- CERT, MSC56-J. - Detect and remove superfluous code and values