Showing 716 of 716 total issues
Define a constant instead of duplicating this literal "' not found" 4 times. Open
logger.error("build '" + jobId + " #" + buildId + "' not found");
- 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.
Extract this nested try block into a separate method. Open
try {
- Read upRead up
- Exclude checks
Nesting try
/catch
blocks severely impacts the readability of source code because it makes it too difficult to understand
which block will catch which exception.
Refactor this method to reduce its Cognitive Complexity from 21 to the 15 allowed. Open
public CIJobsList getJobsList(boolean includeParameters, Long workspaceId) {
- 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
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
Define a constant instead of duplicating this literal "Failed" 4 times. Open
vUserState.put("Failed", new ArrayList<Number>(0));
- 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.
Define a constant instead of duplicating this literal "Error" 4 times. Open
vUserState.put("Error", new ArrayList<Number>(0));
- 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.
Either re-interrupt this method or rethrow the "InterruptedException" that can be caught here. Open
} catch (IOException | InterruptedException ex) {
- 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 reduce its Cognitive Complexity from 28 to the 15 allowed. Open
private void outputReportFiles(List<String> reportNames, File reportDirectory, TestResult testResult, String title,
- 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
Catch Exception instead of Throwable. Open
} catch (Throwable cause) {
- 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)
Change the visibility of this constructor to "protected". Open
public PollHandler(Client client, String entityId, int interval) {
- Read upRead up
- Exclude checks
Abstract classes should not have public constructors. Constructors of abstract classes can only be called in constructors of their subclasses. So
there is no point in making them public. The protected
modifier should be enough.
Noncompliant Code Example
public abstract class AbstractClass1 { public AbstractClass1 () { // Noncompliant, has public modifier // do something here } }
Compliant Solution
public abstract class AbstractClass2 { protected AbstractClass2 () { // do something here } }
Add a default case to this switch. Open
switch (size % 3) {
- 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
Rename "parameters" which hides the field declared at line 62. Open
List<Map<String, String>> parameters = XPathUtils.toEntities(response.toString());
- Read upRead up
- Exclude checks
Overriding or shadowing a variable declared in an outer scope can strongly impact the readability, and therefore the maintainability, of a piece of code. Further, it could lead maintainers to introduce bugs because they think they're using one variable but are really using another.
Noncompliant Code Example
class Foo { public int myField; public void doSomething() { int myField = 0; ... } }
See
- CERT, DCL01-C. - Do not reuse variable names in subscopes
- CERT, DCL51-J. - Do not shadow or obscure identifiers in subscopes
Remove this unused "projectName" private field. Open
private String projectName;
- 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.
Remove this unused method parameter "aClass". Open
public boolean isApplicable(Class<? extends AbstractProject> aClass) {
- Read upRead up
- Exclude checks
Unused parameters are misleading. Whatever the values passed to such parameters, the behavior will be the same.
Noncompliant Code Example
void doSomething(int a, int b) { // "b" is unused compute(a); }
Compliant Solution
void doSomething(int a) { compute(a); }
Exceptions
The rule will not raise issues for unused parameters:
- that are annotated with
@javax.enterprise.event.Observes
- in overrides and implementation methods
- in interface
default
methods - in non-private methods that only
throw
or that have empty bodies - in annotated methods, unless the annotation is
@SuppressWarning("unchecked")
or@SuppressWarning("rawtypes")
, in which case the annotation will be ignored - in overridable methods (non-final, or not member of a final class, non-static, non-private), if the parameter is documented with a proper javadoc.
@Override void doSomething(int a, int b) { // no issue reported on b compute(a); } public void foo(String s) { // designed to be extended but noop in standard case } protected void bar(String s) { //open-closed principle } public void qix(String s) { throw new UnsupportedOperationException("This method should be implemented in subclasses"); } /** * @param s This string may be use for further computation in overriding classes */ protected void foobar(int a, String s) { // no issue, method is overridable and unused parameter has proper javadoc compute(a); }
See
- CERT, MSC12-C. - Detect and remove code that has no effect or is never executed
Add a private constructor to hide the implicit public one. Open
public static class Lcov {
- 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.
Provide the parametrized type for this generic. Open
Optional opt = tmpConfig.getParameters().stream().filter(p -> TestsToRunConverterBuilder.TESTS_TO_RUN_PARAMETER.equals(p.getName())).findFirst();
- Read upRead up
- Exclude checks
Generic types shouldn't be used raw (without type parameters) in variable declarations or return values. Doing so bypasses generic type checking, and defers the catch of unsafe code to runtime.
Noncompliant Code Example
List myList; // Noncompliant Set mySet; // Noncompliant
Compliant Solution
List<String> myList; Set<? extends Number> mySet;
Merge this if statement with the enclosing one. Open
if (affectedFile.exists()) {
- Read upRead up
- Exclude checks
Merging collapsible if
statements increases the code's readability.
Noncompliant Code Example
if (file != null) { if (file.isFile() || file.isDirectory()) { /* ... */ } }
Compliant Solution
if (file != null && isFileOrDirectory(file)) { /* ... */ } private static boolean isFileOrDirectory(File file) { return file.isFile() || file.isDirectory(); }
Add a private constructor to hide the implicit public one. Open
public class ScmPluginFactory {
- 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.
Remove this throw statement from this finally block. Open
throw new ReportParseException(e);
- Read upRead up
- Exclude checks
Using return
, break
, throw
, and so on from a finally
block suppresses the propagation of any
unhandled Throwable
which was thrown in the try
or catch
block.
This rule raises an issue when a jump statement (break
, continue
, return
, throw
, and
goto
) would force control flow to leave a finally
block.
Noncompliant Code Example
public static void main(String[] args) { try { doSomethingWhichThrowsException(); System.out.println("OK"); // incorrect "OK" message is printed } catch (RuntimeException e) { System.out.println("ERROR"); // this message is not shown } } public static void doSomethingWhichThrowsException() { try { throw new RuntimeException(); } finally { for (int i = 0; i < 10; i ++) { //... if (q == i) { break; // ignored } } /* ... */ return; // Noncompliant - prevents the RuntimeException from being propagated } }
Compliant Solution
public static void main(String[] args) { try { doSomethingWhichThrowsException(); System.out.println("OK"); } catch (RuntimeException e) { System.out.println("ERROR"); // "ERROR" is printed as expected } } public static void doSomethingWhichThrowsException() { try { throw new RuntimeException(); } finally { for (int i = 0; i < 10; i ++) { //... if (q == i) { break; // ignored } } /* ... */ } }
See
- MITRE, CWE-584 - Return Inside Finally Block
- CERT, ERR04-J. - Do not complete abruptly from a finally block
Define a constant instead of duplicating this literal "Bytes" 3 times. Open
averageThroughputResultsGraphSet.put(Y_AXIS_TITLE, "Bytes");
- 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.