Mutant Builder - Good or Bad?

[James Kennard]

Hi,

A code base in which I have been working recently, contains a large number of builders that I have dubbed "Mutant Builders". I thoroughly dislike them. I'm looking for two things, (1) some opinions on whether or not the Mutant Builders are in actuality Good or Bad, (2) some good ways of justifying their goodness or badness depending on your point of view.

I'll start with a simple example based on a different domain to that which I'm working on. Imagine we have a Car class and Car consists of an Engine, a Chassis, and a bunch of other related things. The Car class contains a default constructor and a bunch getters and setters (another gripe with this particular code base). Then there is a CarBuilder, and it looks a little like this:

public class CarBuilder {

    private Car car;

    public CarBuilder (Car car) {

        this.car = car;

    }

    public CarBuilder () {

        this.car = new Car();

    }

    public CarBuilder Chassis(Chassis chassis) {

        car.setChassis(chassis);

    }

    public EngineBuilder getEngine() {

        Engine engine = car.getEngine();

        if (engine == null) {

            engine = new Engine();

            car.setEngine(engine);

        }

        return new EngineBuilder(engine);

    }

    // A load more withX and getX type methods

    public Car build() {

        // Clones this.car

        return clonedCar;

    }

}

I'm hoping at this stage I'm not the only scratching my head wondering what just happened. I'll try and talk through the example and highlight the problems as I see them.

1. The CarBuilder contains a Car. I would have expected the CarBuilder to contain all the parts of the Car, and create the Car within the build() method.
2. You can construct the CarBuilder with an existing Car. This means the CarBuilder is mutating an object that not only it has access to, this seems dangerous. Does the caller that instantiated the CarBuilder realise this? Does the CarBuilder realise that the parts it's putting together could be being changed by something else simultaneously?
3. The CarBuilder contains a default constructor. This is less scary, but still highlights the first point.
4. The Chassis method allows you to set the Chassis part. For some reason we've broken with Java lowerCamelCase method naming convention. I would have used withChassis. And again the implementation is highlighting the first point.
5. getEngine returns an Engine builder (in the same ilk as CarBuilder). This relies on the fact EngineBuilder uses the same approach as CarBuilder and mutates the Engine it is passed during construction. This seems particularly bizarre because it means I never call build on the EngineBuilder, and I never explicitly get to tell the CarBuilder with Engine I want my Car to have.
6. The build method uses Object.clone to "build" the Car. Apart from the fact I always steer (no pun intended) clear of clone, this seems at odds with the implementation. So I've potentially given it a Car (depending on which constructor I have used) and it can mess around with that Car, but at the end I get a different (although logically equal) Car. Have I missed something or is this peculiar?
7. The EngineBuilder also has a build method that clones the Engine. But there is no point in me calling the EngineBuilder's build method as I don't get to explicitly tell the CarBuilder which Engine I want. Plus the CarBuilder never uses the EngineBuilder.build() method because it relies on the fact EngineBuilder is mutating the Engine it just created and assigned to the Car.

I have to admit, this made my head swim for a while, especially because the examples I was looking at included an unhealthy degree of inheritance in the builders (up to 6 levels deep in places), a scattering of less than helpful Generics, and next to zero test coverage. But I'll try not to rant.

The only thing I can think that this offers is the fact that Car could have subtypes, and you therefore don't necessarily need a different type of Builder for each subtype.

Any views on this would be gratefully received.

Thanks

James.