Java Rules: “equals(Object obj)” should test argument type

Because the equals method takes a generic Object as a parameter, any type of object may be passed to it. The method should not assume it will only be used to test objects of its class type. It must instead check the parameter’s type.

Noncompliant Code Example

public boolean equals(Object obj) {
  MyClass mc = (MyClass)obj;  // Noncompliant
  // ...
}

Compliant Solution

public boolean equals(Object obj) {
  if (obj == null)
    return false;

  if (this.getClass() != obj.getClass())
    return false;

  MyClass mc = (MyClass)obj;
  // ...
}
Advertisements

Java Rules: “equals” methods should be symmetric and work for subclasses

A key facet of the equals contract is that if a.equals(b) then b.equals(a), i.e. that the relationship is symmetric.

Using instanceof breaks the contract when there are subclasses, because while the child is an instanceof the parent, the parent is not an instanceof the child. For instance, assume that Raspberry extends Fruit and adds some fields (requiring a new implementation of equals):

Fruit fruit = new Fruit();
Raspberry raspberry = new Raspberry();

if (raspberry instanceof Fruit) { ... } // true
if (fruit instanceof Raspberry) { ... } // false

If similar instanceof checks were used in the classes’ equals methods, the symmetry principle would be broken:

raspberry.equals(fruit); // false
fruit.equals(raspberry); //true

Additionally, non final classes shouldn’t use a hardcoded class name in the equals method because doing so breaks the method for subclasses. Instead, make the comparison dynamic.

Further, comparing to an unrelated class type breaks the contract for that unrelated type, because while thisClass.equals(unrelatedClass) can return true, unrelatedClass.equals(thisClass) will not.

Noncompliant Code Example

public class Fruit extends Food {
  private Season ripe;

  public boolean equals(Object obj) {
    if (obj == this) {
      return true;
    }
    if (obj == null) {
      return false;
    }
    if (Fruit.class == obj.getClass()) { // Noncompliant; broken for child classes
      return ripe.equals(((Fruit)obj).getRipe());
    }
    if (obj instanceof Fruit ) {  // Noncompliant; broken for child classes
      return ripe.equals(((Fruit)obj).getRipe());
    }
    else if (obj instanceof Season) { // Noncompliant; symmetry broken for Season class
      // ...
    }
    //...

Compliant Solution

public class Fruit extends Food {
  private Season ripe;

  public boolean equals(Object obj) {
    if (obj == this) {
      return true;
    }
    if (obj == null) {
      return false;
    }
    if (this.getClass() == obj.getClass()) {
      return ripe.equals(((Fruit)obj).getRipe());
    }
    return false;
}

See

  • CERT, MET08-J. – Preserve the equality contract when overriding the equals() method

Java Rules: “catch” clauses should do more than rethrow

catch clause that only rethrows the caught exception has the same effect as omitting the catch altogether and letting it bubble up automatically, but with more code and the additional detrement of leaving maintainers scratching their heads.

Such clauses should either be eliminated or populated with the appropriate logic.

Noncompliant Code Example

public String readFile(File f) {
  StringBuilder sb = new StringBuilder();
  try {
    FileReader fileReader = new FileReader(fileName);
    BufferedReader bufferedReader = new BufferedReader(fileReader);

    while((line = bufferedReader.readLine()) != null) {
      //...
  }
  catch (IOException e) {  // Noncompliant
    throw e;
  }
  return sb.toString();
}

Compliant Solution

public String readFile(File f) {
  StringBuilder sb = new StringBuilder();
  try {
    FileReader fileReader = new FileReader(fileName);
    BufferedReader bufferedReader = new BufferedReader(fileReader);

    while((line = bufferedReader.readLine()) != null) {
      //...
  }
  catch (IOException e) {
    logger.LogError(e);
    throw e;
  }
  return sb.toString();
}

or

public String readFile(File f) throws IOException {
  StringBuilder sb = new StringBuilder();
  FileReader fileReader = new FileReader(fileName);
  BufferedReader bufferedReader = new BufferedReader(fileReader);

  while((line = bufferedReader.readLine()) != null) {
    //...

  return sb.toString();
}

See

Java Rules: “File.createTempFile” should not be used to create a directory

Using File.createTempFile as the first step in creating a temporary directory causes a race condition and is inherently unreliable and insecure. Instead, Files.createTempDirectory (Java 7+) or a library function such as Guava’s similarly-named Files.createTempDirshould be used.

This rule raises an issue when the following steps are taken in immediate sequence:

  • call to File.createTempFile
  • delete resulting file
  • call mkdir on the File object

Note that this rule is automatically disabled when the project’s sonar.java.source is lower than 7.

Noncompliant Code Example

File tempDir;
tempDir = File.createTempFile("", ".");
tempDir.delete();
tempDir.mkdir();  // Noncompliant

Compliant Solution

Path tempPath = Files.createTempDirectory("");
File tempDir = tempPath.toFile();

Java Rules: “Externalizable” classes should have no-arguments constructors

An Externalizable class is one which handles its own Serialization and deserialization. During deserialization, the first step in the process is a default instantiation using the class’ no-argument constructor. Therefore an Externalizable class without a no-arg constructor cannot be deserialized.

Noncompliant Code Example

public class Tomato implements Externalizable {  // Noncompliant; no no-arg constructor

  public Tomato (String color, int weight) { ... }
}

Compliant Solution

public class Tomato implements Externalizable {

  public Tomato() { ... }
  public Tomato (String color, int weight) { ... }
}

Java Rules:”Exception” should not be caught when not required by called methods

Catching Exception seems like an efficient way to handle multiple possible exceptions. Unfortunately, it traps all exception types, both checked and runtime exceptions, thereby casting too broad a net. Indeed, was it really the intention of developers to also catch runtime exceptions? To prevent any misunderstanding, if both checked and runtime exceptions are really expected to be caught, they should be explicitly listed in the catch clause.

This rule raises an issue if Exception is caught when it is not explicitly thrown by a method in the try block.

Noncompliant Code Example

try {
  // do something that might throw an UnsupportedDataTypeException or UnsupportedEncodingException
} catch (Exception e) { // Noncompliant
  // log exception ...
}

Compliant Solution

try {
  // do something
} catch (UnsupportedEncodingException|UnsupportedDataTypeException|RuntimeException e) {
  // log exception ...
}

or if runtime exceptions should not be caught:

try {
  // do something
} catch (UnsupportedEncodingException|UnsupportedDataTypeException e) {
  // log exception ...
}

Java Rules: “HostnameVerifier.verify” should not always return true

To prevent URL spoofing, HostnameVerifier.verify() methods should do more than simply return true. Doing so may get you quickly past an exception, but that comes at the cost of opening a security hole in your application.

Noncompliant Code Example

SSLContext sslcontext = SSLContext.getInstance( "TLS" );
sslcontext.init(null, new TrustManager[]{new X509TrustManager() {
  public void checkClientTrusted(X509Certificate[] arg0, String arg1) throws CertificateException {}
  public void checkServerTrusted(X509Certificate[] arg0, String arg1) throws CertificateException {}
  public X509Certificate[] getAcceptedIssuers() { return new X509Certificate[0]; }

}}, new java.security.SecureRandom());

Client client = ClientBuilder.newBuilder().sslContext(sslcontext).hostnameVerifier(new HostnameVerifier() {
  @Override
  public boolean verify(String requestedHost, SSLSession remoteServerSession) {
    return true;  // Noncompliant
  }
}).build();

Compliant Solution

SSLContext sslcontext = SSLContext.getInstance( "TLSv1.2" );
sslcontext.init(null, new TrustManager[]{new X509TrustManager() {
  @Override
  public void checkClientTrusted(X509Certificate[] arg0, String arg1) throws CertificateException {}
  @Override
  public void checkServerTrusted(X509Certificate[] arg0, String arg1) throws CertificateException {}
  @Override
  public X509Certificate[] getAcceptedIssuers() { return new X509Certificate[0]; }

}}, new java.security.SecureRandom());

Client client = ClientBuilder.newBuilder().sslContext(sslcontext).hostnameVerifier(new HostnameVerifier() {
  @Override
  public boolean verify(String requestedHost, SSLSession remoteServerSession) {
    return requestedHost.equalsIgnoreCase(remoteServerSession.getPeerHost()); // Compliant
  }
}).build();