22

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?
  • 6
    The Builder is going to need to set values somehow. So it doesn't solve the basic problem. – Amy Blankenship Apr 5 '16 at 21:51
  • 3
    What's the reason the (more/dangerous/exception-throwing) stuff can't be moved to before the call to setA? – yatima2975 Apr 6 '16 at 9:41
  • 2
    Only 3 lines of constructor parameters? That's not that bad. – pjc50 Apr 6 '16 at 15:15
  • 7
    In any software design, there are always tradeoffs between complexity and decoupling. Although I think I agree with you personally that builder is a good solution here, your coworker is right to hesitate on the grounds of KISS. I maintain software that is so amazingly over-complicated, and part of the reason is because every new hire goes rogue and says "This is stupid, I'm going to do this new thing my way", hence we have 5 frameworks for scheduling background jobs and 8 methods for querying a database. All is good in moderation and there is no clear right answer in cases like this. – Brandon Apr 6 '16 at 15:36
  • 3
    KISS is a fuzzy concept, modifiable objects are source of complexity hugely underestimated by developers, they make multi-threading, debug and exceptions handling much more difficult than immutable one. Either the object is valid or an exception is created at the construction (which better place to catch that exception?). – Alessandro Teruzzi Apr 6 '16 at 16:35
46

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.

  • 7
    "Surprise is only good if it involves a party," that's not actually true for everyone!! I'd stop talking to any so called friend that threw me a surprise anything, specially a party. With people. Ugh. – Darkhogg Apr 6 '16 at 12:51
  • 3
    So if every refactoring and design pattern, or every decision like "Do I use accessor and mutator here or builder pattern?" has to be approved by a team, how will developers ever feel empowered to do the right thing? How will you make forward-thinking changes if everything must be designed by a committee. I have been in the OP's shoes before where I am stuck having to maintain senseless code (as the new guy) which everybody else was afraid to change because it's consistent. Consistency is good and all, but not at the expense of improvement. – Brandon Apr 6 '16 at 12:56
  • 11
    @Brandon no, but every wide-reaching change that will affect them should. If you have to fix a bug, its just another bit of code - no big deal. If you make a decision that affects everyone else, then its at least politeness to tell them what you're doing. You can get agreement from the team to change your senseless code, and then your changes becomes the new consistency. You just have to talk to your colleagues. There's nothing bad about saying "I'm going to change the crappy accessor pattern we all hate" and hear your colleagues respond with "yeah, its about time someone dealt with that" – gbjbaanb Apr 6 '16 at 13:14
  • 13
    @Brandon there's a saying: "The road to hell is paved with good intentions". Consistency isn't just good, it's necessary! Imagine trying to manage a team of developers who collectively manage 20 applications, each of which is written in a different style and/or architecture because preferences, current technology, trends, etc. dictated what was best at the time. Getting the team to agree that something should change can be the difference between the new guy being accepted because they fixed a long standing problem, and being fired for going cavalier with what you thought was best. – DrewJordan Apr 6 '16 at 13:28
  • 3
    @Brandon - If you are going to go rogue and fix something "that everybody agrees is wrong" then you'd probably have more success by choosing something that "everybody agrees is wrong" rather than choosing something that at best has different camps on the topic;like immutability. A good design/architecture makes immutability high cost for practically no reward. So unless your team has been having problems because they are using setters then you weren't fixing a problem at all. OTOH, if this has been a source of frequent bugs then it seems like the ramifications justify a team decision. – Dunk Apr 6 '16 at 18:52
21

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.

  • You are absolutely right. A small objection: It's not the "old way" vs "my new, super awesome new way". I'm fully aware that I might be talking nonesense. My problem is that by continuing to use software practices everyone in the company finds horrible, I might stagnate as a developer. Thus I try to introduce some change, even if I'm wrong. (The task was triggered by a request to refactor a related class) – rath Apr 6 '16 at 9:09
  • @rath Are there no new codebases being developed? – biziclop Apr 6 '16 at 9:34
  • @biziclop There are new modules being plugged into the old codebase. I worked on one recently. Happy days. – rath Apr 6 '16 at 10:39
  • 5
    @rath Perhaps you need a personal project which you can work on in your own time where you can advance your own development? I'm not convinced by your "Builder" pattern argument either; the getters/setters seem to be a perfectly OK solution based on the information you've provided. Remember that you are accountable to your employer and your team; your priority is making sure that any work you do is of benefit to those you are accountable to. – Ben Cottrell Apr 6 '16 at 10:59
  • @rath Well, even if you are not in a "old/new" code, what matters is how it is perceived by the receiving end, especially if they have more decision power than you. It looks as-if you want to introduce change for your own interest and not for the product you develop. If everyone in the company find the practice horrible, it will eventually change, but this does not look like it is a priority. – coredump Apr 6 '16 at 11:55
17

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.

  • It's a big data holder of Strings and other primitives. There's no logic anywhere, not even checking for nulls etc. So it's just sheer size, not complexity per se. I thought doing something like builder.addA(a).addB(b); /*...*/ return builder.addC(c).build(); would be a lot easier on the eyes. – rath Apr 6 '16 at 9:00
  • 8
    @rath: It's a big data holder of Strings and other primitives. => then you have other problems, hope no-one cares if the e-mail field accidental gets assigned with the guy's post address... – Matthieu M. Apr 6 '16 at 11:40
  • @MatthieuM. One problem at a time I suppose :) – rath Apr 6 '16 at 12:03
  • @rath - It sure seems like incorporating a good validation checking scheme would have been far more useful and more readily received than trying to figure out how to ramshackle the builder pattern into already existing code. Somewhere else you said you want to better yourself. Learning how to get maximum gain for least effort and consequences is definitely an attribute that should be developed. – Dunk Apr 6 '16 at 19:14
  • 1
    I have soooooo many constructors with way more parameters than that. Necessary in IoC patterns. Bad generalization in this answer. – djechlin Apr 6 '16 at 22:03
16

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.

  • 2
    Frame your thoughts as suggestions and ideas. Get THEM to talk through THEIR ideas before asking questions to make them think about yours +1 for that nonetheless – rath Apr 6 '16 at 8:44
  • 2
    @rath Keep in mind that other people might not be reading from your book. It's possible that the person you're having the discussion with regards it as a confrontation, which could be counter-productive to your aims. – Iker Apr 6 '16 at 9:57
  • 2
    @rath: There is no right or wrong. Just trade offs. And more often than not the trade offs involve more than code alone. – Marjan Venema Apr 6 '16 at 11:23
  • 1
    @Dunk All that exist are trade-offs. We call them right or wrong when the trade-offs are crystal clear and obviously on one side. However recognizing when that shades into ambiguous is easier if we focused on them being trade-offs from the start. – btilly Apr 6 '16 at 19:24
  • 2
    There is not a single programming "best practice" that I'm aware of which doesn't have exceptions where it is not best. Sometimes it is hard to find those exceptions, but they always exist. – btilly Apr 6 '16 at 19:26
11

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.

7

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)

  • I don't know the details of your situation but if you came in and tried to change everything without first gaining people's confidence with your skills or not taking the time to learn "why" they do things the way they do then you were treated exactly as you would be treated at just about any company, even really good ones. Actually, especially at really good ones. Once people have confidence in someone's skills then it is simply a matter of making a compelling case for your suggestions. If you can't make a compelling case then there's a good chance your suggestion isn't all that good. – Dunk Apr 6 '16 at 19:22
  • 1
    idk what to reply to your assumptions @Dunk I was not trying to change everything, I tried to clean it up. And it was not people that knew what they where doing, they proudly created classes and functions that where hundred lines of code, with idk how many states, global and so on.. the level was closer to kinder garden imo. So I stand by my statement. Never back down when you notice your self being in a position where you will have to adopt to bad practice to fit in. – superhero Apr 6 '16 at 22:32
  • @Dunk Try to change how people code as a team member is not what I do. But I tell them from the start; "I wont work in this project with out completely rewriting it", if the code is a piece of trash that is. If that's not appropriated, well... then it's mutual. It's not just about a comfort, it's about the fact that I need to be able to take responsibility for what I produce. I can't do that if the surrounding code in the project is a complete mess. – superhero Apr 6 '16 at 22:35
  • You don't have to "forever" work with code that is trash or follow bad practices simply because that's how it is being done. However, if you want to be successful at changing things then you had better prove your credibility first as a minimum. Just about every developer thinks code they inherit is trash. If every company just agreed with the new guy every time without even knowing their true skill level then the trash will turn into a dump. Also, you should take the time to understand why things are done as they are. Sometimes the "wrong" way is the "right" way for a specific situation. – Dunk Apr 8 '16 at 14:03
  • @Dunk I see that you have a different opinion then I in this matter. Yours seem to be more popular, if reading from the other answers. I don't agree, why I wrote this answer in opposition. What you have stated has not changed my mind. – superhero Apr 8 '16 at 15:17
2

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.

  • 1
    by turning the creation of an object into a process, you can enforce complex logical constraints. BAM. And all that this implies about complexity when it is organized into smaller, discrete, simpler steps. – radarbob Apr 10 '16 at 1:22
2

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.

  • It's definitely not design by committee which is part of the reason why the code stinks so bad. Too much of a good thing I suppose. It is a DTO but I won't break it up into pieces; if the codebase is bad the DB is much, much worse. +1 (primarily) for the last paragraph. – rath Apr 6 '16 at 19:30

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.