Let’s Fix Some Crap Code

Let’s suppose we have a database where customers are identified by a composite key. This key contains two integers, a customer ID and district ID. Thankfully, both fields have NOT NULL constraints in the database.

Throughout the Java code people pass multiple parameters, the customerId and districtId, whenever they wish to identify a customer. To fix this problem, some clever programmer writes a CustomerKey class that combines both fields. The code looks something like this:

public class CustomerKey {
    private Integer customerId;
    private Integer districtId;

    public CustomerKey() {
        this(0,0);
    }

    public CustomerKey(Integer customerId, Integer districtId) {
        setCustomerId(customerId);
        setDistrictId(districtId);
    }

    public Integer getCustomerId() {
        return customerId;
    }

    public void setCustomerId(Integer customerId) {
        if (customerId == null) {
            this.customerId = 0;
        } else {
            this.customerId = customerId;
        }
    }

    public Integer getDistrictId() {
        return districtId;
    }

    public void setDistrictId(Integer districtId) {
        if (districtId == null) {
            this.districtId = 0;
        } else {
            this.districtId = districtId;
        }
    }

    public boolean equals(Object o) {
        ...
    }

    public int hashCode() {
        ...
    }
}

Eric Burke’s Code Review

Here are my observations on this class:

1. Using Integer instead of int makes no sense in this case.
2. Silently converting null to 0 is a really bad idea.
3. Because this class is mutable, it is not thread-safe

Why Type Wrappers?

Type wrappers can contain numeric and null values. Primitives can only contain numeric values. Since our database has NOT NULL constraints, there is no reason to allow null in the Java-side.

By allowing null, particularly in a fundamental class like this, you introduce bad code to every piece of application logic that uses this class. Because type wrappers might be null, every programmer utilizing the CustomerKey must guard against NullPointerException.

In this case, use primitives.

The Silent Conversion

The programmer attempts to mask null values like this:

public void setCustomerId(Integer customerId) {
  if (customerId == null) {
    this.customerId = 0;
  } else {
    this.customerId = customerId;
  }
}

Do you like my blog? If so, why not subscribe?

I am guilty of a similar sin several years ago. In a very fundamental piece of framework code that converts values to/from strings, I made the mistake of silently converting null to zero. Now, as many as seven years later, we still see artifacts from this bad design decision. It is very low-level and is deeply intertwined in code that cannot be changed. This is one of my biggest programming mistakes, ever.

Silent data conversions like this are a bad, bad mistake.

This Class Should Be Immutable

Since this class has both getters and setters, it is not thread-safe. Additionally, if you use instances of this class as keys in a Map or as values in a Set, you risk corrupting the data structure if someone modifies the objects after adding them to the collection.

In classes intended to serve as composite keys, it is far better to simply make the class immutable. This reduces many types of errors and further simplifies the code.

A Better Implementation

Here is how I’d write the class:

public class CustomerKey2 {
    private final int customerId;
    private final int districtId;

    public CustomerKey2(int customerId, int districtId) {
        this.customerId = customerId;
        this.districtId = districtId;
    }

    public int getCustomerId() {
        return customerId;
    }

    public int getDistrictId() {
        return districtId;
    }

    public boolean equals(Object o) {
        ...
    }

    public int hashCode() {
        ...
    }
}

Now the class:

• Is more concise and thus easier to read and maintain
• Is immutable and thread-safe
• Cannot contain illegal null values
• Does not do any automatic conversions from null to zero.

What are your thoughts?

This entry was posted on Thursday, September 13th, 2007 at 8:38 pm and is filed under Java. You can skip to the end and leave a response.