Design Patterns – How to Promote the Builder Pattern in a Team

builder-patterndesign-patternsteamwork

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?

Best Answer

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.

Related Topic