Comparison between two design battle approaches

Question

I've recently been learning about design patterns. Now I decided that I know enough to be able to refactor my inventory code. The question solely exist so I could show my fellow programmers the kind of destructive and lame mindset you'd get out of OOP, SOLID etc. Adder.Add<CoolAdd>(new CoolAddArgs(1, 2, 3, etc) instead of the simple function CoolAdd(1, 2, 3, etc) is just absurd.

In an inventory, there are items. Wherever there are items, there's adding and swapping items. In my inventory, there's more than one way to add or swap items - NormalAdd, ForceAdd, MouseSwapAdd, etc. SnapSwap, HoldSwap, etc. (Video)

Currently, I have all those methods in my ItemsBag. I decided to move them outside, to an Adder/Swapper.

I came up with an interesting system, which I think is SOLID. It's a mixture of the strategy and a bit of factor method patterns.

Usage:

Adder adder = new Adder();
adder.Add<NormalAdd>(new NormalAddArgs(1, item, slot));
adder.Add<ForceAdd>(new ForceAddArgs(index));

Swapper swapper = new Swapper();
swapper.Swap<SnapSwap>(new SnapSwapArgs(1, 2, 3, etc));
swapper.Swap<HoldSwap>(new HoldSwapArgs("", 1));

The idea, is that you have a Strategy that does something, given an Args. Now Adder and Swapper are StrategyImplementers.

• Adder is an AddingStrategy implementer.
• NormalAdd and ForceAdd are AddingStrategy.
• Swapper on the other hand, is a SwappingStrategy implementer.

Different strategies have different arguments, that's why a custom Args object has to be created.

Diagram:

Code:

(This is the main part - the bottom left part of the diagram)

public abstract class Args
{

}

public abstract class Strategy
{
    public abstract bool Implement(Args args);
}

public static class StrategyCreator
{
    static public T Create<T>() where T : Strategy, new()
    {
        return new T();
    }
}

public abstract class StrategyImplementor<T> where T : Strategy
{
    private Dictionary<Type, T> dic = new Dictionary<Type, T>();

    protected bool Implement<TStrategy>(Args args) where TStrategy : T, new()
    {
        Type type = typeof(TStrategy);
        T strategy = GetStrategy(type);
        if (strategy == null) {
            strategy = StrategyCreator.Create<TStrategy>(); // lazy
            dic[type] = strategy;
        }
        return strategy.Implement(args);
    }

    private T GetStrategy(Type type)
    {
        T strategy;
        return dic.GetValue(type, out strategy);
    }
}

(Here's the adding stuff - swapping is pretty much the same)

public class Adder : StrategyImplementor<AddingStrategy>
{
    public bool Add<T>(AddArgs args) where T : AddingStrategy, new()
    {
        return Implement<T>(args);
    }
}

/* <<< Adding Strategies >>> */
#region 
public abstract class AddingStrategy : Strategy
{
}

public class NormalAdd : AddingStrategy
{
    public override bool Implement(Args args)
    {
        if (!(args is NormalAddArgs))
            throw new Exception("NormalAdd: Invalid args!");
        Console.WriteLine("Normal add");
        return true;
    }
}

public class ForceAdd : AddingStrategy
{
    public override bool Implement(Args args)
    {
        if (!(args is ForceAddArgs))
            throw new Exception("ForceAdd: Invalid args!");
        Console.WriteLine("Force add");
        return true;
    }
} 
#endregion

/*<<< Strategy Args >>> */
#region
public abstract class AddArgs : Args
{
}

public class NormalAddArgs : AddArgs
{
    public int row;
    public int col;
    public Etc etc;

    public NormalAddArgs() { }
    public NormalAddArgs(int row, int col, etc)
    {
        this.row = row;
        this.col = col;
    }
}

public class ForceAddArgs : AddArgs
{
    public int id;
    public Etc etc;

    public ForceAddArgs(int id, etc)
    {
        this.id = id;
    }
}
#endregion

So as you can see it's very easily to extend and create new strategies without affecting any existing code. All I have to do to add a new CoolAdd strategy is write CoolAdd and CoolAddArgs and that's it! Now I can:

Adder.Add<CoolAdd>(new CoolAddArgs(1, 2, 3, etc);

The one thing I didn't like about it, is imagine how many arrows will come out of the consumer - i.e. the class that's gonna use the code in the Usage section above. It's gonna have to know about all these stuff... Adder, NormalAdd, ForceAddArgs, etc

Me and my buddy (J), who is helping me with my design patterns study, decided that there has to be something done to reduce dependency, maybe to be able to write Adder.Add<NormalAdd>(1, "string", item, etc);. But there wasn't a clear way of doing so...

But then he told me, why not just create an Adder with the adding methods inside, and then when the user wants to add/extend the existing behavior, they he just extends Adder and add his methods there!

public class Adder
{
   public void NormalAdd() { }
   public void ForceAdd() { }
   etc
}

Later on:

public class MyAdder : Adder
{
   public void CoolAdd() { }
}

I care so much about extending the behavior, without modifying existing code because I will be shipping this inventory. If a user does that and I release an update, his changes will be overwritten. I think both our systems guard against that.

Now, my friend's system was very easy to overlook for me because I was so focused on implementing/finding use of design patterns.

I thought a lot about which system is better and why, here are my thoughts in terms of:

1. Single responsibility (S in SOLID):

   I think my system wins, since I have very focused and highly cohesive modules, that each does a very specific thing and nothing else. In his system, it's not very focused. In real code, there might be some adding methods, that does a lot of calculations that they require some helper methods, where would you put those helper methods then? What if ForceAdd had helper1, helper2 and helper3? Where would you put those? In the Adder itself? Do so and the S is broken. In my system, any ForceAdd-related stuff, is in the ForceAdd.

2. Open/closed principle (O in SOLID):

   There is no doubt that my system follows this rule to the bone! How about his? Well, even though every time a new adding method needs to be added, the Adder programmer has to go and add the method to the Adder thus he's modifying the Adder in a way. But I don't think the O is broken here, since he's not modifying any of the pre-existing adding methods. If a user wants to add extra adding methods, he'd extend the original adder and add his methods there, not affecting anything.

3. Liskov substitution principle (L in SOLID):

   Who cares? let's light some flares! I didn't really know how to compare the two in terms of L.

4. Interface segregation principle (I in SOLID):

   I think my system wins again. It's true we don't have interfaces, but the idea behind I, is not to carry extra luggage that you ain't gonna need (YAGNI). In his system, if I wanted to add CoolAdd I'd have to make a new adder that extend the original adder just so that I could use CoolAdd. In other words, I don't really care about the other adding methods that my new adder will have, due to inheritance from the original adder! Extra luggage.

5. Dependency inversion principle (D in SOLID):

   I'm not sure, but I think we both play nice with this. Since we both allow the user to inject the parameters needed for something to work.

6. Simplicity/Less complexity:

   Obviously, my system if far more complex and sophisticated.

7. Less/fewer dependencies and loose coupling:

   He wins here again, since there's very little that the consumer has to know about things. In fact, the consumer only knows about Adder - adder.NormaAdd(1, 2, etc);.

8. Less garbage:

   That's an obvious win for him, since I got a lot of newing going on.

9. Less code, fewer classes, less bothersome:

   Obviously, in his system you write less code, fewer class thus you're less bothered. But is that an important factor?

10. Testing:

    I'm not a unit testing expert, but from what I know, when you make separate focused modules that have single responsibilities, they are easier to test - So it's easy to test each strategy and see how it behaves, since it's a separate object. In his system, all the adding methods is in one class - the Adder. I'm not sure if that will make it hard to test...

11. Readability:

    Both are readable, I guess.

12. Maintainability:

    I think mine is easy to maintain since it's divided to separate components. In his, the Adder could get messy if a lot of adding methods were added...

13. Performance:

    I can't tell without benchmarking, but I'm gonna give this to him. Since I take some time in my casts and newing.

14. Re-Usability:

    I think I currently fail here, since if I wanted to create an adding strategy that takes the same arguments that another adding strategy takes, I can't do that. I have to create a custom args object. So I can't make use of my existing args. In fact, I should look to fix that. I'm not sure about his system...

Summary:

________________________________________
|                           | V  | J   |
----------------------------------------
|S                          | 1  | 0   |
|O                          | 1  | 0.5 |
|L                          | ?  | ?   |
|I                          | 1  | 0   |
|D                          | 1  | 1   |
|Simplicty/Less complexity  | 0  | 1   |
|Less depe/Loose coupling   | 0  | 1   |
|Less garbage               | 0  | 1   |
|Less code, fewer classes   | 0  | 1   |
|Testing                    | 1  | 0   |
|Readability                | 1  | 1   |
|Maintainability            | 1  | 0   |
|Performance                | 0  | 1   |
|Reusability                | 1  | 0.5 |
|______________________________________|
|Final Score                | 8  | 8   |

1. What do you think about the two approaches?
2. Which system would you use? Which system you think is best and why?
3. Are my assessments correct?
4. Is there an important factor I missed that will make a big difference?
5. Is there a disadvantage to either systems that you see I missed?

Answer 1

As an exercise, I think your evaluation is fair - by all means, carry on. I think it's worth mentioning, though, that in terms of production, the factors you've enumerated aren't all weighted equally.

At the end of the day, if your code is totally DRY, you can go home happy. DRY (don't repeat yourself) implies that every datum about how your system works is located in exactly one place in your source. You can't make readable, DRY code difficult to extend - there's only one place to make a change. The "code inertia" as I like to call it, as measured in keystrokes per behavior difference, is at its minimum.

Beyond that point, any addition of complexity should be driven by necessity only. If the system outside of your adders and swappers evolves such that the simpler design cannot logically suffice, that's when you can think about picking apart the simplest thing that could possibly work. If your simpler design is DRY, there is minimal risk in leaving it alone. Complexity, on the other hand, is always a cost - it's work expanding to fill available time, and it's more time you have to spend training someone when you get promoted before they can test/maintain/extend your code.

Since I don't see egregious repetition in either design, I would therefore prefer the simpler one.

Answer 2

Try creating unit tests first that define what it is you need to accomplish. You need unit tests to prove that your code and ideas work anyway, so why not start with them? Writing the tests first will give you incentive to keep your code simple and testable - if you find the tests difficult to write, or are confusing in how you have to use your class, then you've actually exercised this interface you've created, and you will have learned of its potential deficiencies.

Once you've written the unit tests, you should be able to post them here. We could use them as a walk-through document that says "if the user wants to rotate items normally, this is how we accomplish it. If the user changes rotation strategies, this is how we do that.

Unit tests make for very readable, very practical documentation. If I find it hard to understand a unit test, the class is too hard to use. If I have to know a lot of magic setup things to use the class, it's too hard to use. And if it's too hard to use, it's too hard to reuse, and fails the SOLID principles.

Answer 3

I think I came up with a way to halve dependencies, and reduce the amount of code required to add items to just:

AddItems(new NormalAdd(1, 2, 3, etc));

I don't know how I missed this, but the key is to leave the Implement method abstract in the base Strategy class parameterless, and introduce the necessary arguments in the concrete classes themselves!

Usage:

Rotate(new NormalRotate(1, 2, "", etc);
Rotate(new SmoothRotate( stuff );
Rotate(new SoftRotate( other stuff );

public bool Rotate(RotationStrategy s)
{
   return s.Implement();
}

A lot simpler eh?

public abstract class SimpleStrategy
{
    public abstract bool Implement();
    public static T Create<T>() where T : SimpleStrategy, new()
    {
        return new T();
    }
}
public abstract class RotateStrategy : SimpleStrategy { }
public class NormalRotate : RotateStrategy
{
    public int arg1 { protected set; get; }
    public float arg2 { protected set; get; }
    public string arg3 { protected set; get; }

    public NormalRotate() { }
    public NormalRotate(int arg1, float arg2, string arg3)
    {
        Set(arg1, arg2, arg3);
    }

    public NormalRotate Set(int arg1, float arg2, string arg3)
    {
        this.arg1 = arg1;
        this.arg2 = arg2;
        this.arg3 = arg3;
        return this;
    }

    public override bool Implement()
    {
        // do something with arg1, arg2 and arg3
        Console.WriteLine("rotating normally");
        return true;
    }
}
public class SmoothRotate : RotateStrategy
{
    public string[] args { private set; get; }

    public SmoothRotate(string[] args)
    {
        Set(args);
    }
    public SmoothRotate() { }

    public SmoothRotate Set(string[] args)
    {
        this.args = args;
        return this;
    }

    public override bool Implement()
    {
        // do something with args
        Console.WriteLine("smooth rotation");
        return true;
    }
}
public class SoftRotate : SmoothRotate
{
    public int i { private set; get; }

    public SoftRotate() { }
    public SoftRotate(string[] args, int i) : base(args)
    {
        this.i = i;
    }

    public SoftRotate Set(string[] args, int i)
    {
        Set(args);
        this.i = i;
        return this;
    }

    public override bool Implement()
    {
        // do something with i
        Console.WriteLine("damn we're soft!");
        return base.Implement();
    }
}

Why the return this; in the setters? Well, the only thing that I don't like now about my system, is that every time I add/rotate/swap/whatever, I still have to new up stuff. Previously, I had the strategies cached in a nice lazy way. Wouldn't it be nice to have this kind of thing here as well?

We can do this:

var cache = new Cache<SimpleStrategy>();
var normal = cache.Request<NormalRotate>();
RotateItem(normal.Set(1, 2, ""));
// Or
RotateItem(cache.Request<SoftRotate>().Set(new[] { "" }, 10));

Code:

public class Cache<T> where T : SimpleStrategy
{
    private List<T> list = new List<T>();
    public void Add(T strategy)
    {
        list.Add(strategy);
    }
    public void Add(T[] strategies) // if you don't wanna be lazy
    {
        foreach (var strategy in strategies) {
            Add(strategy);
        }
    }
    public void Remove(T strategy)
    {
        list.Remove(strategy);
    }
    public TStrategy Request<TStrategy>() where TStrategy : T, new()
    {
        var strategy = list.FirstOrDefault(s => s.GetType() == typeof(TStrategy)) as TStrategy;
        if (strategy == null) {
            strategy = SimpleStrategy.Create<TStrategy>(); // lazy
            Add(strategy);
        }
        return strategy;
    }
}

So it's up to the user really. Do you wanna cache and allocate/deallocate memory less often? Using the cache, do you want less keystorkes? Instantiate a new strategy each time you want one.

This implementation solves most if not all the problems my previous implementation had:

1. Less garbage
2. Less keystrokes, less code, fewer classes
3. Less dependencies
4. More reusable
5. Less complex

About resuability - notice how SoftRotation inherits SmoothRotation. Why not if it uses the same arguments? This is a problem we had before, when I said what if ForceAdd requires the same arguments as NormalAdd? I previously had to create a separate arguments object for it. Now I don't; I just inherit.

New summary:

________________________________________
|                           | V  | J   |
----------------------------------------
|S                          | 1  | 0   |
|O                          | 1  | .5  |
|L                          | ?  | ?   |
|I                          | 1  | 0   |
|D                          | 1  | 1   |
|Simplicty/Less complexity  | .5 | 1   |
|Less depe/Loose coupling   | .5 | 1   |
|Less garbage               | .5 | 1   |
|Less code, fewer classes   | .5 | 1   |
|Testing                    | 1  | 0   |
|Readability                | 1  | 1   |
|Maintainability            | 1  | 0   |
|Performance                | .5 | 1   |
|Reusability                | 1  | .5  |
|______________________________________|
|Final Score                |10.5| 8   |

I guess that's a win for me.

But no system is perfect. The only thing I don't like about it now, is the fact that Set is not abstracted - each concrete strategy has to write its own. I would rather have Set an abstract method inside SimpleStrategy but since the arguments are different, I would have to rollback to creating argument objects. Then we'd have something similar to what we had before: Set(Args) and then each strategy define it's own args.

It's less complex, but less robust as well. Pretty much a matter of give and take.

Answer 4

I've totally missed something else! The simpler design is actually MUCH better had I considered extension methods! Don't know how I missed that, since I cared so much about "extending" the behavior... with this, I could "KISS" my fancy diagrams goodbye!

public class Adder
{
   public bool NormalAdd(arg1, arg2, etc) { }
   public bool ForceAdd(arg1, etc) { }
   etc...
}

When somebody wants to add something, it's as simple as adding an extension method!

public static class AdderExtensions
{
   public static bool SwapAdd(this Adder a, arg1, etc) { }
}

Usage:

var adder = new Adder();
adder.NormalAdd(stuff);
adder.ForceAdd(stuff);
adder.SwapAdd(stuff);