Adobe-Consulting-Services/acs-aem-commons

View on GitHub

Showing 1,632 of 1,632 total issues

Remove this unused method parameter "bytes".
Open

    protected synchronized EnumMap<ReportColumns, Object> trackDetailedActivity(String item, String action, String description, Long bytes) {

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

Define and throw a dedicated exception instead of using a generic one.
Open

  public void traverseTags(ActionManager manager) throws Exception {

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

Provide the parametrized type for this generic.
Open

            final Map map = new HashMap();

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;

Replace the synchronized class "Hashtable" by an unsynchronized one such as "HashMap".
Open

        final Dictionary<String, Object> properties = new Hashtable<>();

Early classes of the Java API, such as Vector, Hashtable and StringBuffer, were synchronized to make them thread-safe. Unfortunately, synchronization has a big negative impact on performance, even when using these collections from a single thread.

It is better to use their new unsynchronized replacements:

  • ArrayList or LinkedList instead of Vector
  • Deque instead of Stack
  • HashMap instead of Hashtable
  • StringBuilder instead of StringBuffer

Even when used in synchronized context, you should think twice before using it, since it's usage can be tricky. If you are confident the usage is legitimate, you can safely ignore this warning.

Noncompliant Code Example

Vector cats = new Vector();

Compliant Solution

ArrayList cats = new ArrayList();

Exceptions

Use of those synchronized classes is ignored in the signatures of overriding methods.

@Override
public Vector getCats() {...}

Remove this unused method parameter "callbackRegistry".
Open

                           DisposalCallbackRegistry callbackRegistry) {

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

Use already-defined constant 'PROP_DISPATCHER_FILTER_ENGINE_SLING' instead of duplicating its value here.
Open

            if ("sling".equals(filterEngine)) {

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.

Add a nested comment explaining why this method is empty, throw an UnsupportedOperationException or complete the implementation.
Open

    public void processForReplication(Session session, ReplicationAction action) 

There are several reasons for a method not to have a method body:

  • It is an unintentional omission, and should be fixed to prevent an unexpected behavior in production.
  • It is not yet, or never will be, supported. In this case an UnsupportedOperationException should be thrown.
  • The method is an intentionally-blank override. In this case a nested comment should explain the reason for the blank override.

Noncompliant Code Example

public void doSomething() {
}

public void doSomethingElse() {
}

Compliant Solution

@Override
public void doSomething() {
  // Do nothing because of X and Y.
}

@Override
public void doSomethingElse() {
  throw new UnsupportedOperationException();
}

Exceptions

Default (no-argument) constructors are ignored when there are other constructors in the class, as are empty methods in abstract classes.

public abstract class Animal {
  void speak() {  // default implementation ignored
  }
}

Remove this unused method parameter "request".
Open

    public DefaultChecksumGeneratorOptions(SlingHttpServletRequest request) {

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

Provide the parametrized type for this generic.
Open

    private Map<String, Optional<Class>> headerTypes;

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;

Refactor this method to reduce its Cognitive Complexity from 16 to the 15 allowed.
Open

    public Comparator<Node> createComparator(HttpServletRequest request) {

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

Provide the parametrized type for this generic.
Open

    public static Map<String, FieldComponent> getFormFields(Class source, SlingScriptHelper sling) {

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;

Provide the parametrized type for this generic.
Open

        Class basicType = getCollectionComponentType(field);

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;

Rename this method to not match a restricted identifier.
Open

    private void record(String path, String action, String description) {

Even if it is technically possible, Restricted Identifiers should not be used as identifiers. This is only possible for compatibility reasons, using it in Java code is confusing and should be avoided.

Note that this applies to any version of Java, including the one where these identifiers are not yet restricted, to avoid future confusion.

This rule reports an issue when restricted identifiers:

  • var
  • yield
  • record

are used as identifiers.

Noncompliant Code Example

var var = "var"; // Noncompliant: compiles but this code is confusing
var = "what is this?";

int yield(int i) { // Noncompliant
  return switch (i) {
    case 1: yield(0); // This is a yield from switch expression, not a recursive call.
    default: yield(i-1);
  };
}

String record = "record"; // Noncompliant

Compliant Solution

var myVariable = "var";

int minusOne(int i) {
  return switch (i) {
    case 1: yield(0);
    default: yield(i-1);
  };
}

String myRecord = "record";

See

Provide the parametrized type for this generic.
Open

            final Map map = new HashMap();

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;

Change the visibility of this constructor to "protected".
Open

    public <T> AbstractCacheMBean(T implementation, Class<T> mbeanInterface) throws NotCompliantMBeanException {

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
    }
}

Change the visibility of this constructor to "protected".
Open

    public <T> AbstractGuavaCacheMBean(T implementation, Class<T> mbeanInterface) throws NotCompliantMBeanException {

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
    }
}

A "NullPointerException" could be thrown; "processedXmlTempFile" is nullable here.
Open

                Files.deleteIfExists(processedXmlTempFile.toPath());

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

This accessibility update should be removed.
Open

            resourceResolverField.setAccessible(true);

This rule raises an issue when reflection is used to change the visibility of a class, method or field, and when it is used to directly update a field value. Altering or bypassing the accessibility of classes, methods, or fields violates the encapsulation principle and could lead to run-time errors.

Noncompliant Code Example

public void makeItPublic(String methodName) throws NoSuchMethodException {

  this.getClass().getMethod(methodName).setAccessible(true); // Noncompliant
}

public void setItAnyway(String fieldName, int value) {
  this.getClass().getDeclaredField(fieldName).setInt(this, value); // Noncompliant; bypasses controls in setter
}

See

  • CERT, SEC05-J. - Do not use reflection to increase accessibility of classes, methods, or fields

Change this condition so that it does not always evaluate to "false"
Open

            if (caughtException) {

Conditional expressions which are always true or false can lead to dead code. Such code is always buggy and should never be used in production.

Noncompliant Code Example

a = false;
if (a) { // Noncompliant
  doSomething(); // never executed
}

if (!a || b) { // Noncompliant; "!a" is always "true", "b" is never evaluated
  doSomething();
} else {
  doSomethingElse(); // never executed
}

Exceptions

This rule will not raise an issue in either of these cases:

  • When the condition is a single final boolean
final boolean debug = false;
//...
if (debug) {
  // Print something
}
  • When the condition is literally true or false.
if (true) {
  // do something
}

In these cases it is obvious the code is as intended.

See

Refactor this method to reduce its Cognitive Complexity from 17 to the 15 allowed.
Open

    private String handleMatchingPatternAttribute(Pattern pattern, String attrValue) {

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

Severity
Category
Status
Source
Language