Skip to main content
edited body
Source Link
Denis
  • 8.6k
  • 5
  • 33
  • 76

One thinkthing that I really don't like about your code is this

One think that I really don't like about your code is this

One thing that I really don't like about your code is this

Source Link
Denis
  • 8.6k
  • 5
  • 33
  • 76

When I think of immutable object I think of get only properties backed with readonly fields.

public string Bar { get; private set; }

That's not a fully immutable instance you still have the private set but removing the set completely will cause problems in your implementation, because your nested Builder class relies on the fact that it can set them.

What I think is better in this case is to have a private constructor to initalize your get only properties and instead of saving some private instance of the Foo object in the Builder you can save values :

public class Foo
{
    public string Bar { get; }

    public IDictionary<string, string> Corge { get; }

    private Foo(string bar, IDictionary<string, string> corge)
    {
        Bar = bar;
        Corge = corge;
    }

    public class Builder
    {
        private string _bar;
        private readonly IDictionary<string, string> _corge = new Dictionary<string, string>();

        public Builder Bar(string bar)
        {
            _bar = bar;
            return this;
        }

        public Builder AddCorge(string key, string value)
        {
            _corge.Add(key, value);
            return this;
        }

        public Foo Build()
        {
            return new Foo(_bar, _corge);
        }
    }
}

Usage :

Foo foo = new Foo.Builder().Bar("bar").AddCorge("key", "value").Build();

The action delegate doesn't quite fit in my solution so I just removed it.

You can add the public empty constructor if you want to. I personally wouldn't like to have static methods to create an immutable object, I prefer everything to be instance based.

One builder to rule them all..

One think that I really don't like about your code is this

var fooBuilder = Foo.Builder.Create();
fooBuilder.Bar("baz");
var foo = fooBuilder.ToFoo();
fooBuilder.Bar("QUUX"); // bam!

You can't use the same builder to create multiple objects and it's not obvious that it will crash unless you check how the Builder class is implemented.

My solution fixes that

var fooBuilder = new Foo.Builder();
fooBuilder.Bar("baz");
var foo1 = fooBuilder.Build();
fooBuilder.Bar("QUUX"); // no bam!
var foo2 = fooBuilder.Build();