rust-unofficial / patterns

A catalogue of Rust design patterns, anti-patterns and idioms
https://rust-unofficial.github.io/patterns/
Mozilla Public License 2.0
8.11k stars 375 forks source link

Is the `Builder` pattern an anti-pattern in Rust due to the `Default` trait? #64

Closed fschutt closed 3 years ago

fschutt commented 6 years ago

I claim that the builder pattern is more or less an anti-pattern and that you should use the Default trait instead. Here's why:

Let's say we have a struct:

pub struct Window {
    pub title: &'static str,
    pub width: usize,
    pub height: usize,
}
  1. The builder pattern produces way too much code on the creators side while not having a significant amount of code reduction on the users side. This is especially visible if the struct has more fields:

Creator:

// Builder pattern
pub struct WindowBuilder {
    __title: &'static str,
    __width: usize,
    __height: usize,
}

impl WindowBuilder {
    pub fn new() -> Self {
        Self {
            __title: "Default title",
            __width: 800,
            __title: 600,
        }
    }

    pub fn with_title(self, title: &'static str) -> Self {
        Self {
            __title: title,
            __width: self.width,
            __title: self.height,
        }
    }

    pub fn with_dimensions(self, width: usize, height: usize) -> Self {
        Self {
            __title: self.title,
            __width: width,
            __title: height,
        }
    }

    pub fn build(self) -> Window {
        Window {
            title: self.title,
            width: self.width,
            height: self.height,
        }
    }
}

// Default pattern: much less code!
impl Default for Window {
    fn default() -> Self {
        Self {
           title: "Default title",
           width: 800,
           height: 600,
        }
    }
}

See how much code we need to construct a window in comparison to the Default trait?

User:

// Default pattern
let window = Window {
     title: "Original title",
    .. Default::default()
};

// Builder pattern: not a significant reduction of usage code!
let window = WindowBuilder::new()
                     .with_title("Original title")
                     .build();
  1. The builder pattern doesn't protect against double-initialization:
let window = WindowBuilder::new()
                     .with_title("Original title")
                     .with_dimensions(800, 600)
                     .with_title("Oops, overwritten title!")
                     .build();

The Default trait protects against that, because you can't initialize the same field twice. The builder pattern simply overwrites the field and you don't get any warning.

  1. The Default trait eliminates the need for the SomethingBuilder struct. The SomethingBuilder struct is an intermediate struct that provides a certain kind of type safety so that you have to call SomethingBuilder.build() to construct a Something out of a SomethingBuilder. All of this is unnecessary if you use the Default trait - less code with essentially the same outcome. The SomethingBuilder has one appropriate use, in my opinion: When you need something to happen in the .build() function and it needs to happen once (although you can implement this in a default() function, too). For example, you need to tell the OS to create a window. This is where it's appropriate to use a builder. However, I've seen the builder pattern to be completely overused, which is why I'm writing this.

Often times when I'm having a struct with many fields that can have default values, it is easier to implement a default trait than to write ten or twenty builder functions. And that is why I claim that the builder pattern is actually an anti-pattern and that you should use the Default trait instead, wherever possible. It should at least be included somewhere in this repository.

softprops commented 6 years ago

I'm sympathetic to this, specifically about code size, but have found default alone to fall short in certain cases.

https://github.com/colin-kiegel/rust-derive-builder helps address the extra coding issue but doesn't make the code size issue go away. It just derives it for you at compile time.

Cases were default alone fell short were roughly cases of exposing public apis. In particular ones where I didn't want to expose the internal API ( fields ) of my structs directly as pub fields to consumers outside my crate. Once you expose those it's hard to evolve an API without making breaking changes.

Builders can often help with ergonomics. Methods which accept Into<> arguments are often more flexible and ergonomic for consumers. You lose those when you expose the ability to create a struct directly with it's internal fields.

For point 2, I'm not sure that's an actual problem in practice. Immutible builders prevent that particular case but mutable builders can see benefit allowing users to initialize fields with default values that make sense per application and have it double initialized with an override for target cases. With default, you take that control out of the users hands. They can not implement a trait like default that's defined in another crate

For point 3 it's actually useful to have a method like build to validate the semantic correctness of fields to ensure structs may only be created with those semantics.

I think it's fair to say that default impls and builders complement each other. It's often useful to use both in some combination. The derive builder crate supports this. In specific cases you may only need one or the other. I don't think either in isolation in all cases is a silver bullet for struct creation.

FraGag commented 6 years ago

Implementing Default might not be feasible if there is no sensible default for at least one field.

The builder pattern doesn't protect against double-initialization

You could protect against this by tracking the initialization of individual fields at the type level, but then adding a new field to the builder becomes a breaking change, which defeats one of the purposes of builders.

burdges commented 6 years ago

I think your WindowBuilder cannot be considered an example of the builder pattern, just senseless code repetition. It does not "separate construction from representation" since the same representation works.

You need some essential transition in a builder pattern that requires changing the representation, normally a data structure's parts must be transformed simultaneously. As a rule, builder outputs are more opaque than their inputs too. This is a builder pattern:

pub struct WindowBuilder {
    pub title: &'static str,
    pub width: usize,
    pub height: usize,
}

impl Default for WindowBuilder {
    fn default() -> Self { ... }
}

impl WindowBuilder {
    pub fn draw(self) -> OpaqueWindowHandle { ... }
}

What if OpaqueWindowHandle is simply struct Opaque(WindowBuilder)? Isn't that silly too but now an honest transition? In practice, you hide data to avoid inconsistencies with other data, so the draw method likely adds or transforms data in a way that permits conflicts with WindowBuilder.

As an aside, there were interesting discussions about private data handling in functional record updates. I think say TmpMmapBuilder { size: 100, ..Default::default() }.make() should work with

pub struct TmpMmapBuilder<T> {
    path: Path,    // not pub
    pub size: usize,
    pub whatever: Whatever,
    PhandomData<[T]>,
}
impl Default for TmpMmapBuilder { .. }
impl TmpMmapBuilder<T> {
    fn make() -> Mmap { ... }
}
burdges commented 6 years ago

I've never used it but maybe the derive_builder crate encourages some anti-pattern here. At least derive_builder without any tweaks looks questionable.

As an aside, I think many builder patterns take some parameters struct with pub elements for all or part of the builder and package them up as a hidden element of the built object, like

pub struct ObjectParams {
   pub foo: Foo,  // pub so user can change them
   pub bar: Bar
}
pub struct Object {
    params: ObjectParams, // No longer pub so user cannot change anything once built
    ...  // Stuff that actually gets built
}
impl Default for ObjectParams { .. }
impl ObjectParams {
    fn build(self, ...) -> Object { ... }
}

This style will become more flexible once Rust adds nameless struct items, so maybe

pub struct ObjectBuilder {
    pub _: ObjectParams, // Makes ObjectBuilder expose all ObjectParams' elements directly
    meow: Meow, // manipulated only indirectly by builder methods
}
brokenthorn commented 6 years ago

You've got an error in your code:

pub fn build(self) -> Window {
   Window {
       title: self.title,
       width: self.width,
       height: self.height,
   }
}

Should have been:

pub fn build(self) -> Window {
    Window {
        title: self.__title,
        width: self.__width,
        height: self.__height,
    }
}
HenryTheCat commented 6 years ago

Sorry to step in. I was looking at the builder pattern and got here. I have a proposal I just used in some code and I wanted to share, even though it may turn out to be a terrible idea (and feel free to say so).

In a nutshell, I used the From trait:

// A struct I want to have built
pub struct MyStruct {
    // A private field
    secret: u32,
}

// Builder for MyStruct
pub struct MyStructBuilder {
    // A public field
    pub open_data: u32,
}

impl From<MyStructBuilder> for MyStruct {
    fn from(builder: MyStructBuilder) -> Self {
        MyStruct {
            secret: builder.open_data,
        }
    }
}

fn main() {
    let builder = MyStructBuilder { open_data: 5 };
    let my_struct = MyStruct::from(builder);
}

MyStructBuilder may implement the Default trait, so we can easily induce that on MyStruct as well:

impl Default for MyStruct {
    fn default() -> Self {
        MyStruct::from(MyStructBuilder::default())
    }
}

If MyTraitBuilder::from can fail, the From trait could be implemented for Result<MyStruct, ErrorType> instead:

// A struct I want to have built
pub struct MyStruct {
    // A private field
    secret: u32,
}

// Builder for MyStruct
pub struct MyStructBuilder {
    // A public field
    pub open_data: i32,
}

impl From<MyStructBuilder> for Result<MyStruct, ()> {
    fn from(builder: MyStructBuilder) -> Self {
        if builder.open_data >= 0 {
            Ok(MyStruct {
                secret: builder.open_data as u32,
            })
        } else {
            Err(())
        }
    }
}

To justify my approach: the point is that, by using public fields in MyStructBuilder, I don't have to write methods to set each field. Still, the fields in MyStruct are private, so only MyStruct can set them. I could write all set_field methods for MyStruct, but maybe I don't want to. The solution is to use the From trait, so that I can access private fields directly.

sergeysova commented 5 years ago

https://github.com/rust-unofficial/patterns/blob/master/patterns/builder.md

zoechi commented 5 years ago

Perhaps the builder pattern doc should mention that this is about initializing private struct fields. There is no point in using builder (or multiple constructors which the builder pattern is supposed to help to avoid) if all fields are public.

ghost commented 4 years ago

A major issue I see with the builder pattern is it seems to violate the DRY principal very badly. You have to repeat every single parameter out 2 or or even 3 times: builder struct field, setter method, and, if it needs to be in the resulting struct, there too.

I'm not sure if it's unique to these particular crates or a broader issue, but both the macros I see getting recommended a lot for reducing code repetition, derive_builder and typed_builder, seem to break code analysis from the perspective of someone useing your crate which, in my opinion, makes them a poor choice if you're planning on anyone else using your crate.

burdges commented 4 years ago

I've done this a couple times:

pub struct WindowBuilder {
    pub title: &'static str,
    pub width: usize,
    pub height: usize,
}

pub struct Window {
    builder: WindowBuilder,  // Builder here avoids field repitition, but its no longer public.
    derived_data: ..,
}

It's becomes tricky whether you want Window: Deref<Target=WindowBuilder> without DerefMut instead of getter methods though, so users loose access to self or &mut self methods on WindowBuilder