Need help understanding the "Builder" design pattern-CodePudding

I am working on refactoring some code I wrote awhile back, trying to make it more SOLID by implementing some design patterns. Specifically, I am trying to use the builder pattern to instantiate a GUI object.

Here is the code for the 'Product':

public class ContainerShell {
    private JFrame mainView = new JFrame("Election Simulator");
    private JTabbedPane tabbedPane = new JTabbedPane();
    private PopulationController populationController;
    private CandidateController candidateController;
    private ElectionController electionController;

    public JFrame getMainView() {
        return mainView;
    }

    public JTabbedPane getTabbedPane() {
        return tabbedPane;
    }
    
    public void addController(Controller uiController) throws RuntimeException {
        switch (uiController.getType()) {
            case POPULATION:
                this.populationController = (PopulationController) uiController;
            case CANDIDATE:
                this.candidateController = (CandidateController) uiController;
            case ELECTION:
                this.electionController = (ElectionController) uiController;
            default:
                throw new RuntimeException("Unknown controller type");
        }
    }
    
    public PopulationController getPopulationController() {
        return populationController;
    }

    public CandidateController getCandidateController() {
        return candidateController;
    }

    public ElectionController getElectionController() {
        return electionController;
    }
    
}

Here is the code for the 'Concrete Builder':

public class GuiBuilder implements Builder {
    public static final Component contentPaddingX = Box.createRigidArea(new Dimension(10,0));
    public static final Component contentPaddingY = Box.createRigidArea(new Dimension(0,10));
    public static final Component borderPaddingX = Box.createRigidArea(new Dimension(20,0));
    public static final Component borderPaddingY = Box.createRigidArea(new Dimension(0,20));
    private ContainerShell voteSimGui = new ContainerShell();
    
    public GuiBuilder() {
        
    }

    @Override
    public void addPopulationController(PopulationController uiController) {
        voteSimGui.addController(uiController);
    }

    @Override
    public void addCandidateController(CandidateController uiController) {
        voteSimGui.addController(uiController);
    }

    @Override
    public void addElectionController(ElectionController uiController) {
        voteSimGui.addController(uiController);
    }

    public ContainerShell build() {
        voteSimGui.getTabbedPane().addTab("Population", voteSimGui.getPopulationController().getPopPane());
        voteSimGui.getTabbedPane().addTab("Candidates", voteSimGui.getCandidateController().getCandPane());
        voteSimGui.getTabbedPane().addTab("Election Results", voteSimGui.getElectionController().getElectPane());
        voteSimGui.getMainView().setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
        voteSimGui.getMainView().getContentPane().add(voteSimGui.getTabbedPane());
        voteSimGui.getMainView().pack();
        voteSimGui.getMainView().setVisible(true);
        return voteSimGui;
    }
    
}

And the code for the 'Abstract Builder' interface:

public interface Builder {
    void addPopulationController(PopulationController popController);
    void addCandidateController(CandidateController candController);
    void addElectionController(ElectionController electController);
}

Now here is the current working build method of the 'Director':

public void buildGUI() {
        GuiBuilder guiBuilder = new GuiBuilder();
        guiBuilder.addPopulationController(ControllerFactory.getPopulationInstance());
        guiBuilder.addCandidateController(ControllerFactory.getCandidateInstance());
        guiBuilder.addElectionController(ControllerFactory.getElectionInstance());
        gui = guiBuilder.build();
    }

Cool, so what's the problem, right? Well, it may be ultimately inconsequential, but this is my actual preferred implementation of that method:

gui = new GuiBuilder()
    .addPopulationController(ControllerFactory.getPopulationInstance())
    .addCandidateController(ControllerFactory.getCandidateInstance())
    .addElectionController(ControllerFactory.getElectionInstance())
    .build();

See how much cleaner that looks? Only problem is JetBrains is complaining about this for whatever reason...and I can't figure out why. The specific error the compiler gives is that it is unable to resolve the token 'addCandidateController' (but it accepts the addPopulationController). So taken together with the fact that the previous implementation worked tells me that this isn't necessarily an issue with this code's functionality so much as it simply doesn't like how I'm trying to chain the methods together. But then how come Java is allowed to do it?! (From another project):

HttpRequest request = HttpRequest.newBuilder()
                    .uri(URI.create("https://api.kraken.com/0/private/Balance"))
                    .header("API-Key", auth.getPubKey())
                    .header("API-Sign", auth.sign("/0/private/Balance", "nonce="   nonceValue))
                    .POST(HttpRequest.BodyPublishers.ofString("nonce="   nonceValue))
                    .build();

Again, that's an example from a different project wherein I implement Java's HttpRequest.Builder; but that implementation works just fine. So what am I missing?

CodePudding user response：

To enable a call chain, return the object being modified, the builder object.

To each of your add… methods, add a final line return this ;. And change the return type from void to your builder class.

By the way, I don’t see the need for your Builder interface. Will you really have multiple implementations?

@Override
public GuiBuilder addPopulationController(PopulationController uiController) {
    voteSimGui.addController(uiController);
    return this ;
}

@Override
public GuiBuilder addCandidateController(CandidateController uiController) {
    voteSimGui.addController(uiController);
    return this ;
}

You can find open source examples of builders in the OpenJDK implementation of Java. See DateTimeFormatterBuilder and HttpRequest.Builder.

CodePudding user response：

The problem is simply that you want the return type of your 'builder setters' to be itself, and for all such 'builder setters' to end in return this;. In other words, replace:

public void addCandidateController(CandidateController uiController) {
    voteSimGui.addController(uiController);
}

with:

public GuiBuilder addCandidateController(CandidateController uiController) {
    voteSimGui.addController(uiController);
    return this;
}

A few important notes though, because you aren't applying these 'patterns' particularly well in these snippets:

You are violating YAGNI all over the place. Why in the blazes make an interface for this builder? Adding layers of abstraction and indirection is an evil. Usually it is the least evil, i.e. a good idea, but that is despite the fact that you're introducing abstractions and layers of indirection. Layers of indirection confuse matters, and inflate the amount of code you have. These are costs. What are you 'buying' by paying these costs? In the case of having interfaces for your builders, the answer is usually: Absolutely nothing.
The usual naming scheme for builders is just the property, not add. add suggests that there is a list, and even then the usual way to go is still not use the word add. The right name is just candidateController, not addCandidateController.
One advantage of builders is that the actual object you 'build' with it can be immutable. If your ContainerShell object is not immutable, then what's the point of the builder? Why have a builder (and the interface, now you have 3x as much code as you really need)? Why not just have:

class ContainerShell {
  public ContainerShell setPopulationController(PopulationController c) {
    this.populationController = c;
    return this;
  }
  public ContainerShell setCandidateController(CandidateController c) {
    this.candidateController = c;
    return this;
  }
  public ContainerShell setElectionController(ElectionController c) {
    this.electionController = c;
    return this;
  }
}

The above code can be used precisely the way you want to:

new ControllerShell()
 .setPopulationController(ControllerFactory.getPopulationInstance())
 .setCandidateController(ControllerFactory.getCandidateInstance())
 .setElectionController(ControllerFactory.getElectionInstance())
 ;

and uses a quarter of the code, a clear winner.

It's best to think about builders as replacing large argument lists. In other words, builders are actually an alternative for a simple method invocation - you use them when said method (or constructor, which is equivalent to a static method in virtually all relevant ways) would need such a large list of arguments that reading code that calls it is confusing. For example, given this method:

public Bridge(int buildYear, int carLanes, int bikeLanes, int walkLanes, int span) { ... }

a call to it would look like: new Bridge(1981, 4, 2, 2, 2005). And anybody reading that has no idea what's happening, and every time you try to write a call to it, you have no idea what to do. That's sign 1 of: Oh maybe a builder is appropriate. Sign 2 is: Actually, for many of these parameters I may want it to be optional (which java doesn't do well, given that all you can do is make a combinatorial explosion of overloads). Builders 'name' the parameters and give you the option of making them optional. In other words, a builder replaces this:

new Bridge(1981, 4, 0, 2, 2005)

with this:

Bridge.builder()
  .buildYear(1981)
  .carLanes(4)
  // we don't call .bikeLanes; it defaults to 0.
  .walkLanes(2)
  .span(2005)
  .builder()

As you can see, the builder is a lot more 'wordy' - so you're paying a cost by using them (more code!). Is what you gain by it worth the price? I'd say so, in this example, slam dunk case. This code is vastly more readable. You also get the option of splitting the work of making a bridge up so that various separate pieces of code each do part of the work (you can't really 'split up' new Bridge(1981, 4, 0, 2, 2005), but you CAN make a BridgeBuilder, call half of the 'setters', and then hand the builder object off to some other method to finish the job. It's rare that you want this, but if that's relevant - builders are great.

Technically builders can be used anytime it feels right to replace a method with it, regardless of what that method is. In practice, 'methods with really long, unwieldy, confusing argument lists' comes up most often when making immutable objects: Given that you need to specify all of the fields in the constructor (no setters; it's an immutable object!), naturally, you get to long lists, which then leads to wanting builders. Most all books, tutorials etc 'shortcut' this logical reasoning and they should not do that. It goes like this:

Immutable objects lead to long argslists.
Long argslists lead to confusion.
Builders replace long argslists and thus avoid the confusion.

Don't skip steps by saying e.g. 'immutables are best made with builders'. That's an oversimplification.

Point is, which 'long argslist' are you replacing here? I don't see it - hence, builder is wrong here. Unless you want to lean into it and change your ContainerShell object to be immutable. Immutability has all sorts of advantages, however, a GUI object doesn't really 'need' any of those advantages, which again gets us back to: I doubt you want a builder for this. I think you just want the setters to return self and forget all about making a builder for this.

Remember, programming style guidelines and principles have only one goal: To make your code easier to maintain (which includes being more flexible in the face of likely future change requests). If you can't imagine how the application of a rule helps that goal, then do not apply it - the rule itself is not inherently a goal on its own.