8
\$\begingroup\$

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:

enter image description here

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?
\$\endgroup\$
  • \$\begingroup\$ Since you acknowledged that you are not sure about D, you should '?' in the comparison table. \$\endgroup\$ – Chandranshu Nov 21 '13 at 9:59
  • 3
    \$\begingroup\$ swapper.Swap<SnapSwap>(new SnapSwapArgs(1, 2, 3, etc)); That's a lot of repetition. \$\endgroup\$ – abuzittin gillifirca Nov 21 '13 at 15:07
  • \$\begingroup\$ Thanks for that! - your statement inspired me a better solution - I came up with it while taking a bath - taking a bath is nice. \$\endgroup\$ – anon Nov 22 '13 at 4:35
  • 1
    \$\begingroup\$ A system should be primarily evaluated on how well it meets the requirements, not how many ticks it gets on a list of design patterns/principles. In your question the actual requirements are very unclear, so it's very difficult for us to evaluate your code. It's clear there's a lot of complexity in your solution (both of them), but without the bigger picture we cannot determine whether this is intrinsic complexity or accidental complexity. \$\endgroup\$ – MattDavey Nov 22 '13 at 10:06
  • \$\begingroup\$ Thanks for your comment. You are right. I think they both kinda meet the requirements (which is, providing an extensible way of adding/swapping items), that's why I wanted to look at other factors to help me determine which should I go for, since they both take me there. "the actual requirements are very unclear" hmm? I thought that was clear, unless you meant something else. You mean the requirements from these systems, or all my requirements in the whole inventory project? \$\endgroup\$ – anon Nov 22 '13 at 12:00
7
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Thanks for your reply. "I would therefore prefer the simpler one" - over the complex system in my question, or the simplified one in my answer, or both? \$\endgroup\$ – vexe Nov 24 '13 at 4:58
  • \$\begingroup\$ I don't think that DRYness is the only measure of good code. \$\endgroup\$ – svick Nov 24 '13 at 11:26
  • 1
    \$\begingroup\$ @svick sure, it's not the only one. There are multiple designs for any given system that are DRY, after all. My point is that it's the point of diminishing returns. DRY code can certainly be an inadequate design in other respects - maybe those concerns will manifest as problems, maybe they won't. If they do, you can permute any DRY design into another with minimal work to address them. vexe: primarily the one in the question - it seems that adding a strategy has become more complicated than adding a method, and it's unclear what that abstraction is protecting me from. \$\endgroup\$ – sqykly Nov 24 '13 at 18:19
  • \$\begingroup\$ "any addition of complexity should be driven by necessity only" True. TBH, if I was a consumer of this system, it's kinda tough to come up with an adding/swapping method, since I'm already covering all the possible ways. I'm not saying it won't happen that a user would come up with something, just saying it's rare. I certainly tend to easily think of stuff I might not need just because I think I 'might' need them (bad habit). I like the simplicity of Adder.AddNormally(stuff) in the simpler design, also like the flexibility in my answer. I really don't know which to choose, very confused. \$\endgroup\$ – vexe Nov 26 '13 at 17:31
4
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Welcome to Code Review, if you have any questions there are a few of us more than happy to give you some links or answers, you can find a few of us in Code Review General Chat \$\endgroup\$ – Malachi Nov 25 '13 at 20:33
  • \$\begingroup\$ +1! I can't believe I did all that pontificating without mentioning TDD/BDD. Good call, John. \$\endgroup\$ – sqykly Nov 26 '13 at 2:58
  • \$\begingroup\$ Thanks for your answer. Unfortunately, unit testing in Unity3d isn't as simple your normal unit tests - there are some plugins that gives you the ability to unit test - but not sure of their effectiveness. I'm not even sure how to unit test this whole adding/swapping thing... I mean, you got an inventory and items the users could mess with by interacting with the inventory (via mouse) - so... Idk. I'm a self learner and still new to TDD. \$\endgroup\$ – vexe Nov 26 '13 at 16:33
  • \$\begingroup\$ ah, you never mentioned you were using Unity3D. I would like to say, most good design principles go out of the window when you're talking about games development. The vast majority of design patterns & practices you read on the 'net were not forged in that fire. Something to bare in mind :) \$\endgroup\$ – MattDavey Nov 26 '13 at 17:17
  • 1
    \$\begingroup\$ I've never used Unity3d, but SharpUnit supposedly works with it. wiki.unity3d.com/index.php?title=SharpUnit Another option you could take would be to test these in a standalone program without trying to complicate it with Unity. TDD is intended to motivate you to creating code without external dependencies, so that would actually fit well here. You could test your ideas without worrying about mice, Unity, or whatever. Once you see that Dependency Injection solves the problem, it kind of hits you how modularity should work in your favor. \$\endgroup\$ – John Deters Nov 26 '13 at 18:58
4
\$\begingroup\$

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.

enter image description here

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.

\$\endgroup\$
  • 1
    \$\begingroup\$ "I guess that's a win for me :)" - Please do not take this as criticism, but I think you need a slight attitude readjustment. It seems you marvel at the novelty of your own code, and there is a little too much ego in there. It is not a competition. A good developer sees every line of code he writes as a burden, not a triumph. He understands that "clever" code is rarely good code, as "clever" often conflicts with maintainability (arguably the most important factor of good code, even more important than correctness). A truly good developer can solve complex problems with simple code.. \$\endgroup\$ – MattDavey Nov 25 '13 at 9:47
  • \$\begingroup\$ I hope you do not take that comment as criticism. Every developer goes through the self-marvel stage at some point - it is a sign of enthusiasm. You have already demonstrated that you are open to honest constructive-feedback by visiting this site :) \$\endgroup\$ – MattDavey Nov 25 '13 at 9:48
  • \$\begingroup\$ I don't know why you interpreted what I said like that - I never meant it that way. I was never looking for ways to make my code better so that I could 'win' - Maybe I used the wrong word. Every programmer has the tendency to like what he writes more - since he spent his time and sweat on it, but if the other code is better then by all means I'll give up mine. I wasn't holding on my code, just because it's mine. \$\endgroup\$ – anon Nov 26 '13 at 16:22
  • \$\begingroup\$ "A good developer sees every line of code he writes as a burden" - well that's the main reason I'm trying to come up with a better system than what I had - I don't think I agree with you in that a good dev thinks immediately of what he writes as a burden. I think it's natural to feel good about yourself, about what you write - But then after some time you learn and improve yourself - Then you automatically feel that your old codes are burden, since your standards are now higher than before cause of what you learn. This should remain a circle. \$\endgroup\$ – anon Nov 26 '13 at 16:47
  • 2
    \$\begingroup\$ Is there a reason that the Strategy implementations can't be re-used as they are, instead of caching their arguments? Other than that, this is a really big improvement over the original, and very much useable. Although, I still think I would rather have had one Adder and one Swapper ahead of schedule. Matter of priorities, I guess. \$\endgroup\$ – sqykly Nov 26 '13 at 19:32
0
\$\begingroup\$

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);
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy