Method compareUnknownFields
has a Cognitive Complexity of 61 (exceeds 50 allowed). Consider refactoring. Open
private boolean compareUnknownFields(
Message message1,
Message message2,
UnknownFieldSet unknownFieldSet1,
UnknownFieldSet unknownFieldSet2,
- Read upRead up
- Create a ticketCreate a ticket
Cognitive Complexity
Cognitive Complexity is a measure of how difficult a unit of code is to intuitively understand. Unlike Cyclomatic Complexity, which determines how difficult your code will be to test, Cognitive Complexity tells you how difficult your code will be to read and comprehend.
A method's cognitive complexity is based on a few simple rules:
- Code is not considered more complex when it uses shorthand that the language provides for collapsing multiple statements into one
- Code is considered more complex for each "break in the linear flow of the code"
- Code is considered more complex when "flow breaking structures are nested"
Further reading
Avoid deeply nested control flow statements. Open
if ((reporter == null) || !reportMatches) {
continue;
}
- Create a ticketCreate a ticket
Avoid deeply nested control flow statements. Open
if ((reportType != ReportType.MATCHED) || reportMatches) {
reporter.report(reportType, message1, message2, immutable(stack, unknownField));
}
- Create a ticketCreate a ticket
Refactor this method to reduce its Cognitive Complexity from 30 to the 15 allowed. Open
boolean compareRepeatedField(
- Read upRead up
- Create a ticketCreate a ticket
- 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 this method to reduce its Cognitive Complexity from 50 to the 15 allowed. Open
private boolean compareWithFieldsInternal(
- Read upRead up
- Create a ticketCreate a ticket
- 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 this method to reduce its Cognitive Complexity from 16 to the 15 allowed. Open
private void appendPath(ImmutableList<SpecificField> fieldPath, boolean leftSide)
- Read upRead up
- Create a ticketCreate a ticket
- 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 this method to reduce its Cognitive Complexity from 22 to the 15 allowed. Open
private boolean matchRepeatedFieldIndices(
- Read upRead up
- Create a ticketCreate a ticket
- 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 this method to reduce its Cognitive Complexity from 60 to the 15 allowed. Open
private boolean compareUnknownFields(
- Read upRead up
- Create a ticketCreate a ticket
- 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 this method to reduce its Cognitive Complexity from 21 to the 15 allowed. Open
private void appendValue(
- Read upRead up
- Create a ticketCreate a ticket
- 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
TODO found Open
// TODO(chrisn): Genericize UnknownFieldType based on value type?
- Create a ticketCreate a ticket
- Exclude checks
A "NullPointerException" could be thrown; "reporter" is nullable here. Open
reporter.report(
- Read upRead up
- Create a ticketCreate a ticket
- 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
A "NullPointerException" could be thrown; "unknownDescriptor" is nullable here. Open
UnknownFieldType unknownType = unknownDescriptor.getFieldType();
- Read upRead up
- Create a ticketCreate a ticket
- 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
Merge this if statement with the enclosing one. Open
if (scope == Scope.PARTIAL) {
- Read upRead up
- Create a ticketCreate a ticket
- 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(); }
Remove this useless assignment to local variable "newIndex". Open
int newIndex = i;
- Read upRead up
- Create a ticketCreate a ticket
- 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
Define and throw a dedicated exception instead of using a generic one. Open
throw new RuntimeException("Unknown ReportType");
- Read upRead up
- Create a ticketCreate a ticket
- Exclude checks
Using such generic exceptions as Error
, RuntimeException
, Throwable
, and Exception
prevents
calling methods from handling true, system-generated exceptions differently than application-generated errors.
Noncompliant Code Example
public void foo(String bar) throws Throwable { // Noncompliant throw new RuntimeException("My Message"); // Noncompliant }
Compliant Solution
public void foo(String bar) { throw new MyOwnRuntimeException("My Message"); }
Exceptions
Generic exceptions in the signatures of overriding methods are ignored, because overriding method has to follow signature of the throw declaration in the superclass. The issue will be raised on superclass declaration of the method (or won't be raised at all if superclass is not part of the analysis).
@Override public void myMethod() throws Exception {...}
Generic exceptions are also ignored in the signatures of methods that make calls to methods that throw generic exceptions.
public void myOtherMethod throws Exception { doTheThing(); // this method throws Exception }
See
- MITRE, CWE-397 - Declaration of Throws for Generic Exception
- CERT, ERR07-J. - Do not throw RuntimeException, Exception, or Throwable
Remove usage of generic wildcard type. Open
public abstract List<?> getValues(UnknownFieldSet.Field field);
- Read upRead up
- Create a ticketCreate a ticket
- Exclude checks
It is highly recommended not to use wildcard types as return types. Because the type inference rules are fairly complex it is unlikely the user of that API will know how to use it correctly.
Let's take the example of method returning a "List<? extends Animal>". Is it possible on this list to add a Dog, a Cat, ... we simply don't know. And neither does the compiler, which is why it will not allow such a direct use. The use of wildcard types should be limited to method parameters.
This rule raises an issue when a method returns a wildcard type.
Noncompliant Code Example
List<? extends Animal> getAnimals(){...}
Compliant Solution
List<Animal> getAnimals(){...}
or
List<Dog> getAnimals(){...}
Missing a Javadoc comment. Open
public Builder treatAsMapUsingKeyComparator(
- Read upRead up
- Create a ticketCreate a ticket
- Exclude checks
Checks for missing Javadoc comments for a method or constructor.The scope to verify is specified using the Scope
class anddefaults to Scope.PUBLIC
. To verify anotherscope, set property scope to a differentscope.
Javadoc is not required on a method that is tagged with the@Override
annotation. However underJava 5 it is not possible to mark a method required for aninterface (this was corrected under Java 6). HenceCheckstyle supports using the convention of using a single{@inheritDoc}
tag instead of all theother tags.
For getters and setters for the property allowMissingPropertyJavadoc
,the methods must match exactly the structures below.
public void setNumber(final int number){mNumber = number;}public int getNumber(){return mNumber;}public boolean isSomething(){return false;}
This documentation is written and maintained by the Checkstyle community and is covered under the same license as the Checkstyle project.
Missing a Javadoc comment. Open
public Builder treatAsMapWithMultipleFieldsAsKey(
- Read upRead up
- Create a ticketCreate a ticket
- Exclude checks
Checks for missing Javadoc comments for a method or constructor.The scope to verify is specified using the Scope
class anddefaults to Scope.PUBLIC
. To verify anotherscope, set property scope to a differentscope.
Javadoc is not required on a method that is tagged with the@Override
annotation. However underJava 5 it is not possible to mark a method required for aninterface (this was corrected under Java 6). HenceCheckstyle supports using the convention of using a single{@inheritDoc}
tag instead of all theother tags.
For getters and setters for the property allowMissingPropertyJavadoc
,the methods must match exactly the structures below.
public void setNumber(final int number){mNumber = number;}public int getNumber(){return mNumber;}public boolean isSomething(){return false;}
This documentation is written and maintained by the Checkstyle community and is covered under the same license as the Checkstyle project.
These nested if statements could be combined Open
if (scope == Scope.PARTIAL) {
continue;
}
- Read upRead up
- Create a ticketCreate a ticket
- Exclude checks
CollapsibleIfStatements
Since: PMD 3.1
Priority: Medium
Categories: Style
Remediation Points: 50000
Sometimes two consecutive 'if' statements can be consolidated by separating their conditions with a boolean short-circuit operator.
Example:
void bar() {
if (x) { // original implementation
if (y) {
// do stuff
}
}
}
void bar() {
if (x && y) { // optimized implementation
// do stuff
}
}