kennybu / designpatterntests Goto Github PK

I've come across your question related to this on StackOverflow a couple times, and also came to your site.
I know this is quite old, but I have some issues with ConditionalBuilder. And maybe they're just me.

First of all, I like the builderActions list. In fact, I have started doing that with all my Fluent Builders, as it saves the actual object creation to the end, when Build() is called. That seems cleaner to me.

However, I don't like the way the ConditionalBuilder grabs the last action from the list. Seeing the array indexer of Length - 1 makes me shudder. :)

I also don't like the way that conditions can be applied to anything. It seems like to me that you want to limit which items can have conditions. Although perhaps I am wrong here. But if you do want it to apply to all, then it seems like to me I should be able to do something like the following:

var ninjaBuilder = NinjaBuilder
   .CreateNinjaBuilder()
   .AtLevel(level)
   .WithShurikens(10)
   .WithSkill("hideinshadows")
      .When(() => level > 5)
      .When(() => SubClass == NinjaSubClass.Shadow)

And of course, this would fail, because if the first condition failed, then my second condition would now be applied to WithShurikens, not WithSkill.

So, I suggest the following changes.

First, add a new class, ConditionBuilder:

    public class ConditionBuilder<T, TBuilder> where T : class where TBuilder : IFluentBuilder<T>
    {
        private readonly TBuilder builder;
        private readonly Action<T> action;


        public ConditionBuilder(TBuilder builder, Action<T> action)
        {
            this.builder = builder;
            this.action = action;
        }

        public TBuilder When(Func<T, bool> condition)
        {
            builder.BuilderActions.Add(ba =>
            {
                if (condition(ba))
                    action(ba);
            });

            return builder;
        }

        public TBuilder AsBuilder()
        {
            return builder;
        }
    }

IFluentBuilder is defined as follows:

    public interface IFluentBuilder<T> where T : class
    {
        List<Action<T>> BuilderActions { get; set; }
        T Build();
    }

Then we make the following changes to your existing code.

public class NinjaBuilder
    {
        private readonly List<Action<Ninja>> _builderActions;

becomes

public class NinjaBuilder : IFluentBuilder<Ninja>
    {
        public List<Action<Ninja>> BuilderActions { get; set; } = new List<Action<Ninja>>();

obviously references to _builderActions would change too.

And finally, we change WithSkill from

  public NinjaBuilder WithSkill(string skill)
        {
            _builderActions.Add(s => s.Skill = skill);

            return this;
        }

to

  public ConditionBuilder<Ninja, NinjaBuilder> WithSkill(string skill)
        {
            return new ConditionBuilder<Ninja, NinjaBuilder>(this, s => s.Skill  = skill);
        }

Note, that this doesn't totally solve my issue above about multiple .When conditions. Well, it does, because they aren't allowed now. :) But if you actually wanted multiple .When's you'd have to do a bit more work.

But now only the WithSkill method can have conditions, because it's the only one that returns a ConditionalBuilder

This seems cleaner to me. And we never actually remove actions from our list. Instead, we actually make a conditional action. Which is kind of what I would expect from looking at how the methods are used.

There is one thing I still don't like. The AsBuilder method in ConditionalBuilder. If you use the WithSkill, but don't supply a condition, then you now have a ConditionalBuilder<Ninja, NinjaBuilder> object, rather than a NinjaBuilder object. So, your next .With or .Build will generate a compile time error. The AsBuilder method fixes that, but now feels a bit clunky:

var ninjaBuilder = NinjaBuilder
   .CreateNinjaBuilder()
   .AtLevel(level)
   .WithShurikens(10)
   .WithSkill("hideinshadows")
   .AsBuilder()
  .Build();

Of course, you could get around that by supplying a When condition such as .When(() => true), but that also seems clunky.