How can I promote the use of the Builder pattern in my team?

Question

Our codebase is old and new programmers, like myself, quickly learn to do it the way it's done for the sake of uniformity. Thinking that we have to start somewhere, I took it upon myself to refactor a data holder class as such:

• Removed setter methods and made all fields final (I take "final is good" axiomatically). The setters were used only in the constructor, as it turns out, so this had no side-effects.
• Introduced a Builder class

The Builder class was necessary because the constructor (which is what prompted refactoring in the first place) spans about 3 lines of code. It has a lot of parameters.

As luck would have it, a team mate of mine was working on another module and happened to need the setters, because the values he required became available at different points in the flow. Thus the code looked like this:

public void foo(Bar bar){
    //do stuff
    bar.setA(stuff);
    //do more stuff
    bar.setB(moreStuff);
}

I argued that he should use the builder instead, because getting rid of setters allows the fields to remain immutable (they've heard me rant about immutability before), and also because builders allow object creation to be transactional. I sketched out the following pseudocode:

public void foo(Bar bar){
    try{
        bar.setA(a);
        //enter exception-throwing stuff
        bar.setB(b);
    }catch(){}
}

If that exception fires, bar will have corrupt data, which would have been avoided with a builder:

public Bar foo(){
        Builder builder=new Builder();
    try{
        builder.setA(a);
        //dangerous stuff;
        builder.setB(b);
        //more dangerous stuff
        builder.setC(c);
        return builder.build();
    }catch(){}
    return null;
}

My teammates retorted that the exception in question will never fire, which is fair enough for that particular area of code, but I believe is missing the forest for the tree.

The compromise was to revert to the old solution, namely use a constructor with no parameters and set everything with setters as needed. The rationale was that this solution follows the KISS principle, which mine violates.

I'm new to this company (less than 6 months) and fully aware that I lost this one. The question(s) I have are:

• Is there another argument for using Builders instead of the "old way"?
• Is the change I propose even really worth it?

but really,

• Do you have any tips for better presenting such arguments when advocating trying something new?

Answer 1

Thinking that we have to start somewhere, I took it upon myself to refactor a data holder class

This is where it went wrong - you made a major architectural change to the codebase such that it became very different to what was expected. You surprised your colleagues. Surprise is only good if it involves a party, the principle of least surprise is golden (even if it involves parties, then I'd know wear clean underpants!)

Now if you thought this change was worthwhile, it would have been much better to sketch out the pseudocode showing the change and present it to your colleagues (or at least whoever has the role closest to architect) and see what they thought before doing anything. As it is, you went rogue, thought the whole codebase was yours to play with as you thought fit. A player on the team, not a team player!

I might be being a little harsh on you, but I have been in the opposite place: check out some code and think "WTF happened here?!", only to find the new guy (its nearly always the new guy) decided to change a load of things based on what he thinks the "right way" should be. Screws with the SCM history too which is another major problem.

Answer 2

As luck would have it, a team mate of mine was working on another module and happened to need the setters, because the values he required became available at different points in the flow.

As described, I don't see what makes the code require setters. I probably don't know enough about the use-case, but why not wait until all the parameters are known before instantiating the object?

As an alternative, if the situation requires setters: offer a way to freeze your code to that setters throw an assertion error (a.k.a. programmer error) if you try to modify fields after the object is ready.

I think removing the setters and using final is the "proper" way of doing it, as well as enforcing immutability, but is it really what you should be spending your time with?

General tips

Old = bad, new = good, my way, your way... this kind of discussion is not constructive.

Currently, the way your object is used follows some implicit rule. This is part of the unwritten specification of your code and your team might think that there is not much risk for other parts of the code of misusing it. What you want to do here is to make the rule more explicit with a Builder and compile-time checks.

In some way code refactoring is tied to the Broken Window theory: you feel it right to fix any problem as you see it (or make a note for some later "quality improvement" meeting) so that the code always looks pleasant to work with, is safe and clean. You and your team have not same idea about what is "good enough", though. What you see as an opportunity to contribute and make the code better, with good faith, is probably seen differently from the existing team.

By the way, your team should not be assumed to be necessarily suffering from inertia, bad habits, lack of education, ... the worst you can do is to project an image of a know-it-all who is there to show them how to code. For example, immutability is not something new, your peers have probably read about it but just because this subject is becoming popular in blogs is not sufficient to convince them to spend time changing their code.

After all, there are endless ways to improve an existing codebase, but it costs time and money. I could look at your code, call it "old" and find things to nitpick about. For example: no formal specification, not enough asserts, maybe not enough comments or tests, not enough code coverage, unoptimal algorithm, too much memory usage, not using the "best" possible design pattern in each case, etc. ad nauseam. But for most points that I would raise, you'd think: that's okay, my code works, there is no need to spend more time on it.

Maybe your team think that what you suggest is past the point of diminishing returns. Even if you had found an actual instance of a bug due to kind of example you show (with the exception) they would probably just fix it once and not want to refactor the code.

So the question is about how to spend your time and energy, given that they are limited? There are continuous changes made to software but generally your idea start with a negative score until you can provide good arguments to it. Even if you are right about the benefit, it is not a sufficient argument by itself, because it will finish in the "Nice to have, one day..." basket.

When you present your arguments, you have to make some "sanity" checks: are you trying to sold the idea or yourself? What if somebody else presented this to the team? Would you find some flaws in their reasoning? Are you trying to apply some general best practice or did you take into account the specifics of your code?

Then, you have be sure not to appear as-if you are trying to fix your team's past "mistakes". It help to show that you are not your idea but can take feedback reasonably. The more you do this the more your suggestions will have an opportunity to be followed.

Answer 3

How can I promote the use of the Builder pattern in my team?

You don't.

If your constructor has 3 lines of parameters, it is too big and too complex. No design pattern will fix that.

If you have a class whose data comes available at different times, it should be two different objects, or your consumer should aggregate the components and then construct the object. They don't need a builder to do that.

Answer 4

You seem more focused on being right than on working well with others. Worse yet, you recognize that what you're doing is a power struggle and yet fail to think through how things are likely to feel to the people whose experience and authority you are challenging.

Reverse that.

Focus on working with others. Frame your thoughts as suggestions and ideas. Get THEM to talk through THEIR ideas before asking questions to make them think about yours. Avoid direct confrontations, and be outwardly willing to accept that getting on with doing things the group's way (for now) is more productive than fighting an uphill battle to change the group. (Though bring things back.) Make working with you a joy.

Do this consistently and you should create less conflict, and find more of your ideas get adopted by others. We all suffer from the illusion that the perfect logical argument will wow others and win the day. And if it doesn't work that way, we think it should.

But that is in direct conflict with reality. Most arguments are lost before the first logical point got made. Even among purportedly logical people, psychology trumps reason. Convincing people is about getting past psychological barriers to being properly heard, and not about having perfect logic.

Answer 5

Is there another argument for using Builders instead of the "old way"?

"When you have a new hammer, everything will look like a nail." - this is what I thought when I read your question.

If I was your coleague, I would ask you "why not initialize whole object in the constructor?" That is what it is it's functionality after all. Why use the builder (or any other design) pattern?

Is the change I propose even really worth it?

It is difficult to tell from that small code snippet. Maybe it is - you and your coleagues needs to evaluate it.

Do you have any tips for better presenting such arguments when advocating trying something new?

Yes, learn more about the topic before discussing it. Arm yourself with good arguments and rationales before entering the discussion.

Answer 6

I been in the situation where I was recruited for my skills. When I got to view the code, well .. it was more then horrible. When then trying to clean it up, someone that has been there longer always suppress the changes, because they don't see the benefits. It sounds like something you are describing. It ended with me quitting that position and I can now evolve much better in a company that has a better practice.

I don't think you should just role along with the others in the team because they have been there longer. If you see your team members using bad practices in the projects you work in, it's a responsibility you have to point it out imo, as you have. If not, then you are not a very valuable resource. If you do point thees things out over and over, yet no one is listening, ask you management for a replacement or change job. It's very important that you don't allow yourself to adapt to there bad practices.

To the actual question

Why use immutable objects? Because you know what you are dealing with.

Say you have a db connection that is passed around that allows you to change the host through a setter, then you don't know what connection it is. Being immutable, then you do know.

Say however that you have a structure of data that can be retrieved from persistence and then re-persisted with new data, then it make sense that this structure isn't immutable. It's the same entity, but the values can change. Instead use immutable value objects as arguments.

What you are focusing the question on is however a builder pattern to get rid of dependencies in the constructor. I say a big fat NO to this. The instance is obviously to complex already. Don't try to hide it, fix it!

Tl;dr

Removing the setters can be correct, depending on the class. The builder pattern is not solving the problem, just hiding it. (Dirt under the carpet still exists even if you can't see it)

Answer 7

While all the other answers are very useful (more useful than mine is going to be), there is a property of builders you haven't mentioned yet: by turning the creation of an object into a process, you can enforce complex logical constraints.

You can mandate for example that certain fields are set together, or in a specific order. You can even return a different implementation of the target object depending on those decisions.

// business objects

interface CharRange {
   public char getCharacter();
   public int getFrom();
   public int getTo();
}

class MultiCharRange implements CharRange {
   final char ch;
   final int from;
   final int to;

   public char getCharacter() { return ch; }
   public int getFrom() { return from; }
   public int getTo() { return to; }
}

class SingleCharRange implements CharRange {
   final char ch;
   final int position;

   public char getCharacter() { return ch; }
   public int getFrom() { return position; }
   public int getTo() { return position; }
}

// builders

class Builder1 {
   public Builder2 character(char ch) {
      return new Builder2(ch);
   }
}

class Builder2 {
   final char ch;
   int from;
   int to;

   public Builder2 range(int from, int to) {
      if (from > to) throw new IllegalArgumentException("from > to");
      this.from = from;
      this.to = to;
      return this;
   }

   public Builder2 zeroStartRange(int to) {
      this.from = 0;
      this.to = to;
      return this;
   }

   public CharRange build() {
      if (from == to)
         return new SingleCharRange(ch,from);
      else 
         return new MultiCharRange(ch,from,to);
   }
}

This is a noddy example that doesn't make much sense but it demonstrates all three properties: the character is always set first, the range limits are always set and validated together, and you choose your implementation at build time.

It's easy to see how this can lead to more readable and reliable user code but as you can also see, there's a downside to it: lots and lots of builder code (and I even left out the constructors to shorten the snippets). So you try to calculate the trade-off, by asking questions like:

• Do we only instantiate the object from one or two places? Is this likely to change?
• Maybe we should enforce these constraints by refactoring the original object into a composition of smaller objects?
• Would we end up passing the builder around from method to method?
• Could we just use a static factory method instead?
• Is this part of an external API, that needs to be as foolproof and slick, as possible?
• Approximately how many of our current backlog of bugs could've been avoided if we had builders?
• How much extra documentation writing would we save?

Your colleagues simply decided that the trade-off wouldn't be beneficial. You should discuss the reasons openly and honestly, preferably without resorting to abstract principles, but concentrating on the practical implications of this solution or that.

Answer 8

It sounds like this class is actually more than one class, put together in the same file/class definition. If it's a DTO, then it should be possible to instantiate it in one go and for it to be immutable. If you don't use all of it's fields/properties in any one transaction, then it's almost certainly breaking the Single Responsibility principle. It might be that breaking it up into smaller, more cohesive, immutable DTO classes might help. Consider reading Clean Code if you haven't already so you are better able to articulate the issues, and the benefits of making a change.

I don't think you can design code at this level by committee, unless you are literally mob programming (it doesn't sound like you're on that kind of team), so I think it's okay to make a change based on your best judgement, and then take it on the chin if it turns out you judged it wrong.

However, as long as you are able to articulate the potential issues with the code as it is, then you can still debate that a solution is required, even if yours is not the accepted one. People are then free to offer alternatives.

"Strong opinions, weakly held" is a good principle - you go ahead with confidence based on what you know, but if you find some new information that counters, it, be modest and step down and enter a discussion on what the correct solution should be. The only wrong answer is to leave it to get worse.