Small decisions matter while coding

While coding, small choices, made with the right intention, can often lead to strange problems some time down the line. I’ve noticed that this usually occurs when you have imperfect knowledge, or when the parts of your systems are not making their intentions clear.

Let me illustrate this with two examples. Both examples interestingly deal with error handling, a topic which I personally feel is where most bugs get shipped.

Imperfect knowledge

We use Dropwizard for our (Java-based) services. Dropwizard is a neat little framework that brings together a bunch of Java libraries in an opinionated manner.

Dropwizard integrates with Hibernate and provides a nice abstraction: @UnitOfWork.

@POST
@UnitOfWork
public Response createPerson(Person person) {
    final Person createdPerson = dao.createPerson(person);
    return Response.status(Response.Status.CREATED)
        .entity(createPerson)
        .build();
}

The documentation says that:

This will automatically open a session, begin a transaction, call [createPerson], commit the transaction, and finally close the session. If an exception is thrown, the transaction is rolled back.

We started to use this feature pretty heavily.

At the same time, we wanted to give custom error messages for some types of errors. So, after a few iterations and a few different people working on the service, the resulting code ended up being something like:

@POST
@UnitOfWork
public Response createPerson(Person person) {
    try {
        final Person createdPerson = dao.createPerson(person);
        return Response.status(Response.Status.CREATED)
            .entity(createPerson)
            .build();
    } catch (PersonValidationError e) {
        // Do some extra processing here
        return Response.status(Response.Status.BAD_REQUEST).build();
    }
}

This looks about correct, right? Wrong.

Since we are catching the exceptions and returning a response, the @UnitOfWork annotation does not think anything went wrong, and the transaction will not be rolled back.

This bug was introduced because we made the assumption that @UnitOfWork will magically do the right thing. Of course, the documentation is explicit about the behaviour of @UnitOfWork, but how often do you look up the documentation for every library or framework feature?

If you didn’t already know about the exact contract of @UnitOfWork, or if you didn’t read the documentation, it’s easy to make this mistake.

(The right way to handle this is to use Jersey’s exception mappers.)

Libraries and frameworks often do a lot of heavy lifting for us, a lot of magic. This sometimes comes at a hidden cost — you may not realise what the right way to use a tool is, until you run into edge cases yourself.

No one can have perfect knowledge. This is where experience helps. I’ve found the best way to combat this problem is to have better communication: if you run into a bug like this, let your team know. Send a slack message to your team, publish it in an internal wiki, or write a blog post about the problem 😀

(Another solution, of course, is to have a better API design that makes the intention more clear. But you can only do this for your own code, and it’s not always possible to be 100% clear when you define an API interface.)

Not making your intention clear

This is a more subtle point. While doing code reviews, I often see code like this:

private boolean isPersonInMetroCity(Person person) {
    if (person == null || person.getAddress() == null) return false;
    return METRO_CITIES_SET.contains(person.getAddress().getCity());
}
public Rupee calculateHouseRentAllowance(
    Person person, RentDetails rentDetails) {
    // Internally calls isPersonInMetroCity
}

I do not like the null checks there. Specifically, this check: person == null

Let me explain further: if the person object is null, that calculating the house rent allowance for the person does not make sense at all. It should be the responsibility of the calling method (calculateHouseRentAllowance) to check for this case, and you should not even need to call isPersonInMetroCity.

Depending on the business use case, the second null check (check whether address is null) may be valid. It depends on whether you want to allow calculation of house rent allowance without an address being entered by the user.

Even in that case, the right answer depends on what the specific business requirement is: if the user does not give you their address, do you assume they are in a metro city or not? Here, we can’t tell if the business requirement was handled correctly or not — the only thing we can tell is that the code will not throw an exception.

This is how I would suggest refactoring that method:

private boolean isPersonInMetroCity(@NotNull Person person) {
    // If there is no address specified, 
    // assume person is not in a metro city.
    // <link to reference / tax law>
    if (person.getAddress() == null) return false;
    return METRO_CITIES_SET.contains(person.getAddress().getCity());
}
public Rupee calculateHouseRentAllowance(
    Person person, RentDetails rentDetails) {
    // Handle the null check for `person` here
}

(This is assuming you’re using Intellij’s annotations.)

The contract is clear: Person cannot be null. Your tooling may warn you about this fact.

The special cases are clear: null address is explicitly treated as ‘Not present in metro city’, with a link to a reference in your project spec or a link to the relevant tax law. Of course, you may not always have the luxury of doing this, but these are the places where documentation really helps.

Instead of annotations, another option is to use assertions in your code. Depending on your tooling, annotations may be useful to give you a compile time warning, while assertions make it very explicit what the behaviour of your method is and may throw runtime exceptions.

Do not shy away from runtime errors though. It may be better to just throw a NullPointerException and fail the request, than to build an API that always returns HTTP 200 OK, but sometimes gives the wrong answer.

Caveat 1: This is of course no replacement for test cases.

Caveat 2: All of this depends on the type of system you’re building. You will have different tradeoffs when building a kernel extension vs. building a REST endpoint.

Both of these are extremely minor points in the larger scheme of things. But they still matter, and will play a large part in defining the quality of your code base over time.

The only way that I’ve found to deal with these issues is regular code reviews, and trying to write about such problems (hence this blog post!).