7
\$\begingroup\$

There are a lot of parameters I would like to configure for my Analysis object, so having some kind of composition helpers is a big thing to simplify proper use of it. Let’s say that we have only 3 parameters for the sake of clarity (there are a way more, but amount of code in C# is overwhelming):

    public double WaterTableDepth { get; private set; }
    public bool UeqProfile { get; private set; }
    public bool UWProfile { get; private set; }  

I would like to keep them in an immutable object with two ways to specify parameters:

  • base new Analysis on an existing one overriding random parameters (builder scenario)
  • Consequently specify all the parameters without a chance to miss one (wizard scenario)

Builder scenario might look like this:

        var analysis1 = Analysis.Default
            .Customize()                
            .UseUeqProfile()
            .IgnoreUWProfile()
            .Build();

Or

        var analysis2 = analysis1
            .Customize()
            .WithWaterTableDepth(200)
            .UseUWProfile()
            .Build();

So order/amount of parameters to override is not under C# control:

enter image description here

Wizard scenario requires all the parameters to be specified in the predetermined order:

        var analysis3 = Analysis.Wizard
            .WithWaterTableDepth(20)
            .UseUeqProfile()
            .IgnoreUWProfile()
            .Build();

C# editor always suggests the next step:

enter image description here

Here is the implementation of the Analysis class:

public partial class Analysis
{
    public static readonly Analysis Default = Wizard
        .WithoutGroundWater()
        .IgnoreUeqProfile()
        .IgnoreUWProfile()
        .Build();

    Analysis()
    {
    }

    Analysis(Analysis source)
    {
        WaterTableDepth = source.WaterTableDepth;
        UeqProfile = source.UeqProfile;
        UWProfile = source.UWProfile;
    }

    public double WaterTableDepth { get; private set; }
    public bool UeqProfile { get; private set; }
    public bool UWProfile { get; private set; }        
}

Where builder API is:

public interface IAnalysisBuilder
{
    IAnalysisBuilder WithWaterTableDepth(double value);
    IAnalysisBuilder WithoutGroundWater();

    IAnalysisBuilder UseUeqProfile();
    IAnalysisBuilder IgnoreUeqProfile();

    IAnalysisBuilder UseUWProfile();
    IAnalysisBuilder IgnoreUWProfile();

    Analysis Build();
}

And defined in Analysis as well:

public partial class Analysis : IAnalysisBuilder
{
    public IAnalysisBuilder Customize() => this;

    IAnalysisBuilder IAnalysisBuilder.WithoutGroundWater() =>
        new Analysis(this) { WaterTableDepth = 1000000 };

    IAnalysisBuilder IAnalysisBuilder.WithWaterTableDepth(double value) =>
        new Analysis(this) { WaterTableDepth = value };

    IAnalysisBuilder IAnalysisBuilder.IgnoreUeqProfile() =>
        new Analysis(this) { UeqProfile = false };

    IAnalysisBuilder IAnalysisBuilder.UseUeqProfile() =>
        new Analysis(this) { UeqProfile = true };

    IAnalysisBuilder IAnalysisBuilder.IgnoreUWProfile() =>
        new Analysis(this) { UWProfile = false };

    IAnalysisBuilder IAnalysisBuilder.UseUWProfile() =>
        new Analysis(this) { UWProfile = true };

    Analysis IAnalysisBuilder.Build() => this;
}

Here comes the wizard API:

public interface IAnalysisWaterTableDepth
{
    IAnalysisUeqProfile WithWaterTableDepth(double value);
    IAnalysisUeqProfile WithoutGroundWater();
}

public interface IAnalysisUeqProfile
{
    IAnalysisUWProfile UseUeqProfile();
    IAnalysisUWProfile IgnoreUeqProfile();
}

public interface IAnalysisUWProfile
{
    IAnalysisWizard UseUWProfile();
    IAnalysisWizard IgnoreUWProfile();
}

public interface IAnalysisWizard
{
    Analysis Build();
}

And implementation (in the same class):

public partial class Analysis : 
    IAnalysisWaterTableDepth,
    IAnalysisUeqProfile,
    IAnalysisUWProfile,
    IAnalysisWizard
{
    public static readonly IAnalysisWaterTableDepth Wizard = new Analysis();

    IAnalysisUeqProfile IAnalysisWaterTableDepth.WithoutGroundWater() =>
        new Analysis(this) { WaterTableDepth = 1000000 };

    IAnalysisUeqProfile IAnalysisWaterTableDepth.WithWaterTableDepth(double value) =>
        new Analysis(this) { WaterTableDepth = value };

    IAnalysisUWProfile IAnalysisUeqProfile.IgnoreUeqProfile() =>
        new Analysis(this) { UeqProfile = false };

    IAnalysisUWProfile IAnalysisUeqProfile.UseUeqProfile() =>
        new Analysis(this) { UeqProfile = true };

    IAnalysisWizard IAnalysisUWProfile.IgnoreUWProfile() =>
        new Analysis(this) { UWProfile = false };

    IAnalysisWizard IAnalysisUWProfile.UseUWProfile() =>
        new Analysis(this) { UWProfile = true };

    Analysis IAnalysisWizard.Build() => this;
}

It is easy to consume, ISP rules, but amount of implementation code is crazy...

\$\endgroup\$
  • \$\begingroup\$ What does this have to do with FP? \$\endgroup\$ – RubberDuck Jul 19 '17 at 1:52
  • \$\begingroup\$ @RubberDuck Above APIs are immutable and have no side effects. "In computer science, functional programming is a programming paradigm—a style of building the structure and elements of computer programs—that treats computation as the evaluation of mathematical functions and avoids changing-state and mutable data." WiKi. \$\endgroup\$ – Dmitry Nogin Jul 19 '17 at 2:05
  • 2
    \$\begingroup\$ I wouldn't say that Immutability and Purity are solely the domain of FP. Typically, it's the user of Higher Order Functions and treating those higher order functions as first class citizens that defines FP. Also, a builder is hardly stateless and is about as OOP as it gets. Regardless, I was really asking what you hope to get by tagging this with FP? \$\endgroup\$ – RubberDuck Jul 19 '17 at 2:08
  • 1
    \$\begingroup\$ @RubberDuck The answer is: this solution is a way closer to FP then Imperative side of The Force, so the tag brings the light on coding style. And I cannot hope to gain anything specific – there should be an open question… :) \$\endgroup\$ – Dmitry Nogin Jul 19 '17 at 2:38
  • 1
    \$\begingroup\$ @t3chb0t Fixed, idea is nice :) \$\endgroup\$ – Dmitry Nogin Jul 19 '17 at 4:28
5
\$\begingroup\$

I feel like this approach isn't particularly helpful. Don't get me wrong, I think the fluent API is cool, and the use of some kind of "workflow interface" is a great idea but :

  1. IMO, a Builder helps to resolve complex object creation. Your object isn't complex to create, it just takes a crazy-boring amount of time to write the code to create it. What I mean is that, for example, if an Analysis Without Ground Water was a different implementation of an Analysis, the building process would worth it because it'd hide all this implementation logic but ATM, you always manipulate an Analysis object so what's the point? It feels like an overhead and a design decision that was made because you wanted to write a cool API! Then again, maybe I'm wrong.

  2. I know nothing about your program, but are all combinations of parameters possible? Aren't there more... regular set of parameters that follow some business logic? If so, you could use inheritance to create subclasses of Analysis and simply set the parameters there.

Example!

public class Analysis 
{
    public double WaterTableDepth { get; private set; }
    //All the code logic

    public Analysis(/*zillion parameters*/) 
    {
    }

}

public class AnalysisForSomeParticularScenario
{
    public AnalysisForSomeParticularScenario() : base(/*set parameters*/) {}
}

Now, if you truly have a lot of parameters, nothing stops you from changing the private setters to protected ones and set the values you want in the constructor of the subclass.

public class Analysis 
{
    public double WaterTableDepth { get; protected set; }
    //All the code logic

}

public class AnalysisForSomeParticularScenario
{
    public AnalysisForSomeParticularScenario() : base() 
    {
        //Set parameters here
    }
}

Now, maybe I'm wrong, maybe there's no logic in the creation of the object and there's nothing to regroup. Still, I have some things to say about your implementation :

var analysis2 = analysis1
        .Customize()
        .WithWaterTableDepth(200)
        .UseUWProfile()
        .Build();

That feels wrong. The building aspect of your code shouldn't be coupled to your logic class. The builder has logic itself, don't make it part of the Analysis class, it'll become huge. Doing so, you could do something like this, which feels much better :

var analysis2 = new MyAnalysisBuilder()
        .FromExisting(analysis1)
        .WithWaterTableDepth(200)
        .UseUWProfile()
        .Build();

To resume, I don't feel like a Builder is the way to go unless you're 100% sure that there's no business logic to encapsulate in your different Analysis parameters combinations. If you stick to the builder, separate it's logic from the Analysis class.

Edit

Considering there's an "unlimited" amount of parameter combinations my previous approach won't work. If you really want a Builder, we'll make one that is separated from the Analysis class. After all, in an OOP perspective, building an Analysis as nothing to do with the Analysis itself! One of the main problem I see now is that considering Analysis itself is a builder, I could do something like this :

var analysis = Analysis.Wizard.WithWaterTableDepth(20);

Now what? I didn't call Build, is my object in a good state? Is this what I want? Who knows!

Your base interface is fine, but Analysis shouldn't be an IAnalysisBuilder for the same reasons an House class isn't an HouseBuilder.

public interface IAnalysisBuilder
{
    IAnalysisBuilder WithWaterTableDepth(double value);
    IAnalysisBuilder WithoutGroundWater();

    IAnalysisBuilder UseUeqProfile();
    IAnalysisBuilder IgnoreUeqProfile();

    IAnalysisBuilder UseUWProfile();
    IAnalysisBuilder IgnoreUWProfile();

    Analysis Build();
}

You claim Intellisense will help you, but you're kind of wrong. If you use an interface, it's because you should expect people to use it. Meaning :

IAnalysisBuilder builder = Analysis.Wizard;
builder.?;

You lost your workflow's progression because IAnalysisBuilder returns IAnalysisBuilder for each method call. IMO, all your subclasses should implement IAnalysisBuilder

public interface IAnalysisWaterTableDepth : IAnalysisBuilder
{
    IAnalysisUeqProfile WithWaterTableDepth(double value);
    IAnalysisUeqProfile WithoutGroundWater();
}

And your IAnalysisBUilder should return to good type of interface according to where you are in the object's creation workflow. I know nothing about your domain so let me make an example :

interface IBuilder
{
    IBuilderWithSomeConstraint BuildWithFoo();
} 

interface IBuilderWithSomeConstraint : IBuilder
{
    IBuilderStepTwo BuildWithBar();
}

//So you can do :
IBuilder builder = new Builder();
//There Intellisense will be a tad more helpful.
builder.BuildWithFoo().BuildWithBar();

As for your builder itself, well, just take the code from the Analysis class that is used to build your object, and put it in a AnalysisBuilder class.

\$\endgroup\$
  • \$\begingroup\$ Totally agree with everything you say in general. It is way better to go with a composition of ValueObject to represent parts of the state in case of having any kind of discovering any kind of structural grouping and specialized Analysis type. The problem is that the business domain is all about math and does not allow structuring in any reasonable way: what I have is a flat list of input parameters (anyone can be tweaked at any moment), square data table in, square data table out. \$\endgroup\$ – Dmitry Nogin Jul 20 '17 at 4:48
  • \$\begingroup\$ All calculations are uniform, just some kind of Double.NaN might go out periodically – but keeping the input/output shape stable helps the end user of the library. What I need is compiler verification on not missing anything new (introduced by maintenance) in the user wizard automation code without losing ability to tweak any individual parameter in the builder scenario (so I do need the list of parameters to be flat). \$\endgroup\$ – Dmitry Nogin Jul 20 '17 at 4:49
  • \$\begingroup\$ So this long flat list is a major problem – how to pass it in case of a dedicated Builder class? We also need a copy of all Analysis properties in a builder itself or we would use Analysis copy to represent state of the builder. First approach basically doubles the code and makes Builder an extended copy of Analysis. Second one brings O(n^2) development complexity – 10 parameters mean 100 arguments to pass (10 Analysis ctor parameters X 10 builder setters). Try to type it in for just a couple of parameters in a realistic manner and you will see it yourself… \$\endgroup\$ – Dmitry Nogin Jul 20 '17 at 4:49
  • \$\begingroup\$ @DmitryNogin why do you need to keep copy of the Analysis properties? As for the builder class itself, if you don't have a choice, I will update my answer regarding the Builder \$\endgroup\$ – IEatBagels Jul 20 '17 at 12:54
  • \$\begingroup\$ @ TopinFrassi Sorry for taking so long to get back to you. Yes, it looks very natural to keep a copy. I would like to fork analyses easily and re-expose immutable to copy in the resultset. \$\endgroup\$ – Dmitry Nogin Jul 31 '17 at 16:03
6
\$\begingroup\$

I have done the wizard one before it's a pain to keep track of all the interfaces and which one returns which method and it's even more painful if you decide to change the API later, I would stay away from it.

The case with the builder is too many constructor arguments makes it unreadable, but we have named and optional arguments now.

new Analysis(
    waterTableDepth: 200,
    ueqProfile: true,
    uwProfile: false
 );

let's say that waterTableDepth is optional with some default value.

new Analysis(
    ueqProfile: true,
    uwProfile: false
);

looking at the parameter info will tell you which arguments are optional and the default value for them. Even if it was 20 arguments I wouldn't mind looking at it.

keeping track of which arguments are mandatory and which are not with builder is not an easy task, but with this it will simply not compile.

\$\endgroup\$
  • \$\begingroup\$ Thanks. Consecutive order of interface declarations helps a lot. I expect more users of the library than library developers, so trade-off is all about making consumption easier as much as possible. I also could have something like IAnalysisWaterTableDepth.WithDefaultWaterTableDepth() to make it explicit in the wizard flow (OK, not this one, for sure :) \$\endgroup\$ – Dmitry Nogin Jul 19 '17 at 5:44
  • \$\begingroup\$ Builder starting point is always valid - so it is OK to be selective on what to change. Also please note that multiple related changes can be done by a single method with multiple parameters, while ctor does not provide such kind of temporarily grouping. \$\endgroup\$ – Dmitry Nogin Jul 19 '17 at 5:50
  • \$\begingroup\$ If a constructor takes a large amount of argument I would probably group related ones into small classes. Divide and conquer (if possible). I don't think C# lends itself well to an interface wizard approach. I also tried it, but it resulted in too much boilerplate code for my taste. \$\endgroup\$ – Pieter Witvoet Jul 19 '17 at 12:09
  • \$\begingroup\$ @PieterWitvoet Agree, a lot of noise around. The same time, consuming is streamlined. Unfortunately, it is pretty common to have a long list of parameters without any kind of natural composition grouping while doing math. It could happen that they might mutate in one ore more groups, but there is no obvious compositional structure supporting all the crossing mutation scenarios... \$\endgroup\$ – Dmitry Nogin Jul 19 '17 at 14:00
2
\$\begingroup\$

First of all, I would stick with builder approach. But points written below also can be applied to wizard.

  1. Analysis shouldn't implement IAnalysisBuilder. Builder definitely should be separate class that constructs Analysis. At now I can write (IAnalysisBuilder)analysis without Customize method. So in fact you have no builder, you have Analysis with some interfaces.

  2. In my opinion private set doesn't mean immutability. Analysis will be immutable if you will remove setters and will set properties only via constructor. As for pain with a lot of parameters of the Analysis constructor (which can be internal and thus hidden from user) - yes, it is pain. But pain for you, not for user. Look at point 3 where I suggest some way to make all this stuff simpler. Also at now with your code I can write (IAnalysisBuilder)analysis and call any public method of "builder" that will change analysis. Do you still think you have immutability? :)

  3. So I suggest to create AnalysisBuilder class with fields that corresponds target properties of Analysis. Methods of a builder will just set these fields. In Build method create new instance of Analysis passing all those builder's fields to Analysis constructor. Or you can create some internal class like AnalysisProperties, set its properties while working with builder and pass customized instance of AnalysisProperties to internal constructor of Analysis setting all properties in constructor.

  4. With your code you have serious problem with Build method: two consequent calls will return the same object. As for me it is absolutely wrong. Again, if I'll get two instances of Analysis via Build method and then will use something like ((IAnalysisBuilder)analysis).UseUWProfile() on first instance the second one will also be changed.

So I would rewrite your code like this:

internal sealed class AnalysisProperties
{
    // ... here all properties that Analysis needs
}

public sealed class AnalysisBuilder
{
    private readonly AnalysisProperties _properties = new AnalysisProperties();

    // ... here methods that set properties of _properties

    public Analysis Build()
    {
        return new Analysis(_properties);
    }
}

public class Analysis
{
    internal Analysis(AnalysisProperties properties)
    {
        // ... set all properties of this Analysis
    }

    // ... here all properties of Analysis
}
\$\endgroup\$
  • \$\begingroup\$ 0. Wizard pays out when you have 20+ parameters. Way easy to forget setting one and constructor invocation becomes too long (there is no natural parameter grouping). \$\endgroup\$ – Dmitry Nogin Jul 19 '17 at 3:55
  • \$\begingroup\$ 1. How AnalysisBuilder could construct Analysis? Copy constructor will not work. Let’s say we have 5 ctor parameters: we will end up with 25 arguments across 5 builder methods. Imagine the maintenance scenario with scattered changes to add/remove parameter. \$\endgroup\$ – Dmitry Nogin Jul 19 '17 at 3:55
  • \$\begingroup\$ 2. Encapsulated setters are OK. Class is immutable as long as there is no mechanism to change it state from outside. \$\endgroup\$ – Dmitry Nogin Jul 19 '17 at 3:55
  • \$\begingroup\$ 3. Try to type it in for 5 parameters. Here is where C++ friend visibility rules – no one expect new ctor to be added to the class without changing it, so being closed for extension would be totally OK… 4. Analysis has Value Semantics. It makes no difference to return the same object as well as object with the same state as long as it stays immutable. \$\endgroup\$ – Dmitry Nogin Jul 19 '17 at 3:56
  • \$\begingroup\$ I understand all of these difficulties but I believe builder should be separate object. Also instead of pairs like IgnoreUWProfile/UseUWProfile methods I would left only UseUWProfile adding bool parameter to it. \$\endgroup\$ – Maxim Jul 19 '17 at 4:08

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.