Misusing the Builder Pattern in Java

Introduction

If you've programmed in Java, you've probably come across the Builder Pattern. If not, you could check out some of the 45 million examples on GitHub or read Frank Kiwy's in-depth explanation Exploring Joshua Bloch's Builder design pattern in Java.

Like most human activities, programming involves recognizing patterns and repeating. I've noticed that developers learn certain patterns without their motivating context and then apply them in less than ideal ways.

It's as if I saw someone use a hammer to nail in a screw, saw that the screw was now firmly attached to the wood, and only ever use a hammer and screws to fasten my wood in all my future woodworking endeavors. What I failed to consider, however, is that the person simply didn't have a screwdriver nor nails and the task needed to get done immediately.

Today, I want to take you through a code review session involving the Builder pattern. It's not the pattern you read about in books though; its a third pattern that I've found in several Java codebases. I had to editorialize quite a bit, but I assure you that this is heavily based on a true story.

In the beginning

I had just ramped up on a new team and was eager to contribute some code. One of my first commits included this code:

final class VirtualHostId {
    public final long groupId;
    public final long hostId;

    VirtualHostId(long groupId, long hostId) {
        this.groupId = groupId;
        this.hostId = hostId;
    }
}

// Calling it
var vhostid = new VirtualHostId(request.getGroupId(), request.getHostId());

Your code smells; use a Builder

What I thought was a simple class to hold two related identifiers, turned out to be some of the most smelliest code according to one of my fellow junior developer who had been on the team for 2.8 years¹.

My first thought was, "Builder pattern? For this?! To quote Bloch in Effective Java:

The Builder pattern is a good choice when designing classes whose constructors or static factories would have more than a handful of parameters, especially if many of the parameters are optional or of identical type.

My next thought was about the whole "we use dependency injection" comment, but I didn't address it at that point because I didn't follow their reasoning. Don't worry though because it comes up again later.

After that, our tech lead was asked to review the review and he disagreed with me. So, I changed the code to what is probably the most degenerate case of the Builder pattern ever:

final class VirtualHostId {

    static class Builder {
        private final long groupId;
        private final long hostId;

        public VirtualHostId build() {
            return new VirtualHostId(this);
        }

        public Builder(long groupId, long hostId) {
            this.groupId = groupId;
            this.hostId = hostId;
        }
    }

    public long getGroupId() {
        return groupId;
    }

    public long getHostId() {
        return hostId;
    }

    private final long groupId;
    private final long hostId;

    private VirtualHostId(Builder builder) {
        groupId = builder.groupId;
        hostId = builder.hostId;
    }
}

// To call it...
var vhostid = new VirtualHostId
        .Builder(request.getGroupId(), request.getHostId())
        .build();

Which Builder pattern did you want?

Shortly after my revision, my tech lead called concerned. "Your Builder is not quite right. It looks like you've just moved your constructor into the Builder class."

Then, like the TPS report scene from Office Space, "have you read Effective Java?"

In fact, I had read it as I had mentioned in my original code review response. I had even reread the Builder chapter to ensure that I wasn't misremembering anything before implementing my code. Especially since I thought it wasn't applicable.

I'll skip the entire play-by-play, but essentially, this was what my lead wanted me to do:

1. Hide all constructors as required by dependency injection and use a factory method to return the Builder
2. Don't require any parameters at first and provide setters for every property, so that they "can change throughout object construction"
3. add a validation step within build() to handle constraints such as required properties.

I told you the DI thing was going to come back. The best I can tell, it was just a very strict interpretation (or misinterpretation) of a Google Guice best practice article, Keep Constructors Hidden:

any direct use of a constructor circumvents Guice's object instantiation

I simply disagreed, "won't we directly use the VirtualHostId's construction within the Builder::build method?"

Regarding the ability to change the properties during construction, my lead gave me an hypothetical example of a countable property. It might need to be called repeatedly. My argument was simply that neither of these properties should be changed once set. It was quickly dismissed.

With that, this was the code that was approved:

final class VirtualHostId {

    static class Builder {
        private Long groupId;
        private Long hostId;

        public Builder setGroupId(long groupId) {
            this.groupId = groupId;
            return this;
        }

        public Builder setHostId(long hostId) {
            this.hostId = hostId;
            return this;
        }

        public VirtualHostId build() {
            validate();
            return new VirtualHostId(this);
        }

        private void validate() {
            if (groupId == null || hostId == null) {
                throw new IllegalStateException("groupId and hostId are required");
            }
        }

        private Builder() {}
    }

    static Builder builder() {
        return new Builder();
    }

    private final long groupId;
    private final long hostId;

    public long getGroupId() {
        return groupId;
    }

    public long getHostId() {
        return hostId;
    }

    private VirtualHostId(Builder builder) {
        groupId = builder.groupId;
        hostId = builder.hostId;
    }
}

// Creating a new object is done like:
…
    var vhostid = VirtualHostId.builder()
            .setGroupId(123)
            .setHostId(456)
            .build();

How has it come to this?

My first question, dear Reader: is this still Joshua Bloch's Builder pattern?

Does this look anything like the Gang of Four Builder pattern which describes the Builder participant as one that "specifies an abstract interface for creating parts of a Product object"?

At the risk of sounding pedantic, I think not and I think that's important. As developers, we need to speak a shared language to make it easier to express our ideas and quickly get to a common understanding. If someone asks you to fasten some screws into their wooden shed, I promise they are not expecting you to use a hammer to do so.

I fully acknowledge that there are appropriate applications to patterns like Builder, but for reasons unknown to me, developers aren't weighing alternatives and even misconstruing such patterns all while claiming that the benefits remain regardless. What's worse is that, even when someone questions it, they quickly dismiss it or even actively argue against it claiming "best practice".

To me, the approved code was unnecessarily complicated. Compare the first version and the last. Which one is easier to read? To understand? To use?

There is no real benefit of using the pattern here over what the language provides. A consequence of using this third Builder is that every class requires the creation of a second class. Furthermore, the getters and setters are unnecessary as the public final modifiers makes the properties read-only. The added validation was only necessary because of the introduction of a parameterless static-creation method that essentially allowed all parameters to be optional. This new Builder really only solves problems of our own creation.

Finally, don't dismiss visibility scopes. One of the retorts against my public fields was that "someone else could come along later and make the class public". By that reasoning, we need to treat all classes as public because at some point they could be made public, and that disregards a huge benefit of visibility.

The end?

Some say software engineering is "programming over time". Our story doesn't end with this single commit. Eventually, people got tired of seeing the same 4-line calling code over and over. So, to keep things DRY, it was enclosed in a helper method:

private VirtualHostId makeVirtualHostId(long groupId, long hostId) {
    return VirtualHostId.builder()
        .setGroupId(groupId)
        .setHostId(hostId)
        .build();
}

// So what would have been this:
var vhostid = new VirtualHostId(request.getGroupId(), request.getHostId());

// Is now:
var vhostid = makeVirtualHostId(request.getGroupId(), request.getHostId());