Removing duplication on null checks

This article describes two ways to remove duplication when classes check that methods of an interface do not return null. I'm not sure which way is better, so let me know what you think!

Suppose we have an interface with two methods that return objects:

public interface Athlete
{
  Height getHeight();
  Weight getWeight();
}

Further suppose that Athlete instances are passed as parameters to many different classes in a system. The classes that accept Athlete instances conform to the fail-fast design philosophy, so they not only check if the instance is null, they also check if the athlete's height and weight are null:

public class AthleteUser
{
  public AthleteUser(final Athlete a)
  {
    if (a == null) throw new IllegalArgumentException();
    if (a.getHeight() == null) throw new IllegalArgumentException();
    if (a.getWeight() == null) throw new IllegalArgumentException();
    // Use athlete instance
  }
}

When more than one class performs this same validation, we have duplication. We don't want to repeat ourselves, so how do we remove this duplication?

One option is to introduce a helper class for dealing with Athelete instances. If we follow the naming conventions of Google Guava, we name the helper class Athletes:

public final class Athletes
{
  private Athletes(){}
  public static void assertNoNullReturnValues(final Athlete a)
  {
    if (a == null) throw new IllegalArgumentException();
    if (a.getHeight() == null) throw new IllegalArgumentException();
    if (a.getWeight() == null) throw new IllegalArgumentException();
  }
}

An alternative approach, one I have seldom seen in production, is to put the helper class inside the interface:

public interface Athlete
{
  Height getHeight();
  Weight getWeight();
  public final class Helper
  {
    private Helper(){}

    public void assertNoNullReturnValues(final Athlete a)
    {
      if (a == null) throw new IllegalArgumentException();
      if (a.getHeight() == null) throw new IllegalArgumentException();
      if (a.getWeight() == null) throw new IllegalArgumentException();
    }
  }
}

Which method do you prefer?

The separate class is used more frequently, but the class-inside-the-interface approach has its advantages:

• It makes it much easier to find the helper class.
• It reduces the number of files.

Organizational Leakage

I had a bad experience this weekend at Tim Horton's. My girlfriend and I ordered the sandwich combos which include a doughnut and drink. The nice lady behind the counter quickly got us our doughnuts and drink, put them on the counter, then proceeded to help the next customer in line. I waited at the counter for our sandwiches, growing increasingly impatient. After many minutes, the same lady that took the order originally asked what I was waiting for. I told her that I was waiting for our two sandwiches and her reply was "oh, okay, that's not something I have to worry about". This annoyed me but what does it have to do with software? I'll get to that.

As a Tim Horton's customer, I don't care that they have decided to separate jobs so that one employee is a counter/drink/doughnut person while another is a sandwich maker. My transaction is with Tim Horton's and their internal organizational structure should be irrelevant to me. Just get me my ham and cheese, please!

The same "organizational leakage" occurs in software when error messages indicate the module at fault. Customers don't care about modules... they bought a product not a bunch of modules!

Code Smell: Classes named Impl

A colleague and I have come to the conclusion that public classes named Impl are a code smell. Such classes are often a sign that not enough thought was put into naming the class. Let's look at an example.

Suppose I have a system that stores information about people in a MySQL database. I need a data access object (DAO) for the people, so I start by creating a PersonDao interface. Even though my system only has one way to store people, an interface is a good idea for the following reasons:

• I might want to introduce another implementation (say an LDAP directory) in the future.
• I want to write unit tests for classes that depend on a PersonDao and an interface makes it easier to mock the dependency. You do write unit tests, right?

So what do I name the class that implements the PersonDao interface and uses a MySQL database? I only have one implementation class, so why not name it PersonDaoImpl?

That's a bad choice for a few reasons:

• It doesn't accurately describe what the class is.
• It leads to inconsistencies when further implementation classes are added in the future. For example, suppose a few years after release we discover that some of our customers would rather have their people stored in an LDAP directory. We can't name the LDAP implementation class PersonDaoImpl because that name is already chosen so we use LdapPersonDao. Now we're left with similar classes with dissimilar names.

A better choice is to name the original MySQL implementation MySqlPersonDao and avoid these problems.

Spend a little time to name your classes accurately. If you're having trouble with that, your classes are probably doing too much and you need to refactor.

The Importance of Overridding Equals

I'm trying to write JUnit tests for some code that uses classes defined in Netscape's Lightweight Directory Access Protocol (LDAP) Software Development Kit (SDK) and I am having difficulties because the authors of several classes did not override equals.

For example, suppose I want to write a JUnit test for a method that returns an LDAPModificationSet. My first attempt is as follows:

public void testGetModifications()
{
  final LDAPModificationSet expected = // some expected modifications
  final LDAPModificationSet actual = classUnderTest.methodUnderTest();
  assertEquals(expected, actual);
}

No matter how hard I try, this test will always fail. LDAPModificationSet does not override equals, so its equals method only returns true when passed the exact same instance.

Consider the alternatives for my test:

• Assert something weaker. For example, instead of assertEquals(expected, actual), I could use assertEquals(expected.size(), actual.size()).
• Write my own method that compares LDAPModificationSet instances. For example, I could create an LdapModificationSets class with a method boolean equals(LDAPModificationSet firstSet, LDAPModificationSet secondSet).
• Have my test manually compare each element in LDAPModificationSet. (Although that's not even possible in this case, because each element is an LDAPModification, and that class also doesn't override equals.)

None of these alternatives are particularly fun.

The moral of this story: Override equals in value objects! The second moral of this story - buy "Effective Java: Second Edition" because Joshua Bloch already made this point in Item 8 when he wrote the book!

How to write the best exception constructors

This article describes how to write the best exception constructors. It is related to Item 63 (Include failure-capture information in detail messages) of Joshua Bloch's tremendous book - "Effective Java: Second Edition". If you haven't got it yet, what are you waiting for?

Let's start by discussing the importance of the string returned from an exception's toString method. When an exception is logged, it is usually accompanied either by its stack trace or by the string returned from its toString method. However, according to the Javadoc of the Throwable.printStackTrace method, the first line of a stack trace "contains the result of the toString() method". Therefore, the most useful debugging aid an exception can offer is a toString method that returns details of anything that contributed to the exception. This article is going to show how to write exception constructors that make it easy to have a great toString method.

There are three main types of exceptions:

• those for general use
• those for a specific condition
• those used as the abstract base class of an exception hierarchy

The guidelines for constructor creation depends on the type.

General-use exceptions

General-use exceptions are intended for use in a variety of situations. Examples of such exceptions are java.lang.IllegalStateException and java.text.ParseException. When an exception has such a high potential for reuse, it's best to mimic the four constructors offered by java.lang.Exception. In particular, for FooException, define the following constructors:

• FooException()
• FooException(String message)
• FooException(Throwable cause)
• FooException(String message, Throwable cause)

Specific exceptions

Exceptions such as java.io.FileNotFoundException are only intended for use in a specific situation. The best approach when writing specific exceptions is to provide constructors that force a useful toString method. For example, write FileNotFoundException as follows:

/**
 * A checked exception that indicates that a file is not found.
 */
public final class FileNotFoundException extends IOException
{
  /**
   * Constructs a new instance of FileNotFoundException.
   *
   * @param file the file that was not found
   */
  public FileNotFoundException(final File file)
  {
    super(getMsg(file));
  }

   private static String getMsg(final File file)
  {
    if (file == null)
    {
      return "null";
    }
    else
    {
      return file.getAbsolutePath();
    }
  }
}

When the constructor is defined to accept a File, the class is easy to use and hard to misuse. All instances return a useful string from their toString method. For example, if file is not null, the return value of toString is com.kblaney.FileNotFoundException: D:\temp\kyle.txt
How's that for a string that includes everything I need to know to debug a problem? The exception's class name tells me what went wrong (a file was not found) and the exception's message provides the absolute path of the file that was not found.

Note that FileNotFoundException has already been defined in Java and its existing constructors can't be removed for backwards-compatibility reasons. However, in this case Java is not something to be emulated. We can do better!

Abstract base class of exception hierarchy

An abstract base class of an exception hierarchy should provide constructors that help its derived classes follow the above rules. If there are many derived classes, it is probably best to mimic the four constructors offered by java.lang.Exception. If there are few derived classes, the abstract base class should mimic the constructors required for any of the derived classes.