9
\$\begingroup\$

I have a drop down list contain "Add" and "Subtract". When user select either one, it will call a function Calculate()

private int Calculate(string sAction)
{
    int iTotal = 0;

    if(sAction == "Add")
    {
        iTotal = X + Y;
    }
    else if(sAction == "Subtract")
    {
       iTotal = X-Y;
    }     

    return iTotal;
}

I do not wish to hard coded it to compare the action. It seem like not fulfill Open Closed Principle. If I change the text in the drop down list, I need to change the function as well. May I know how I can improve this code?

\$\endgroup\$
  • 8
    \$\begingroup\$ Defensive programming tip: Add a final else with exception in case the input is neither 'Add' or 'Substract'. \$\endgroup\$ – Aphelion Oct 10 '17 at 7:22
11
\$\begingroup\$

Be sure when you ever have a code that it has the chain of if else there should be an improvement. (even many programmers say you should not use if statement in your code as possible and some others say don't use else statement at all!)

You can use something like this:

First Add a class to manage your actions:

 public class ActionRegistry: Dictionary<string, Func<int, int, int>>
    {
        public ActionRegistry()
        {
            this.Add("Add", (x, y) => x + y);
            this.Add("Subtract", (x, y) => x - y);
        }

    }

Then you can use that class like this:

public class DoStuff
{
    private int Calculate(string sAction)
    {
        var actionRegistry= new ActionRegistry();
        var a = 1;
        var b = 2;
        //var actionResult= actionRegistry[this should come from your drop down].Invoke(a, b);
        var actionResult= actionRegistry[sAction].Invoke(a, b);


        //you can even Register New Action Like this :
        actionRegistry.Add("Multiply",(x,y)=>x*y);

        //then you can use it somewhere else:
        var multiplyResult =  actionRegistry["Multiply"].Invoke(a, b);
        return actionResult;
    }
}

Every time your action has changed you just need to add the new action in your ActionRegistry. With this approach, there is no need for that if-else statement.

By the way, you can even use interface and DI to loosely couple the ActionManager.

UPDATE Here my update using enums:

First Declare an Enum :

enum ActionType 
{ 
 Add, 
 Subtract 
}

Then use that enum in your ActionRegistry class:

public class ActionRegistry: Dictionary<int, Func<int, int, int>>
{
    public ActionRegistry()
    {
       //it's better to register your actions outside the class
       // and don't use enum inside your class
       // but for simplicity i did that inside the class 

       this.Add((int)ActionType.Add, (x, y) => x + y);
       this.Add((int)ActionType.Subtract, (x, y) => x - y);
    }

}

then you should change your calculate method like this :

 private int Calculate(int actionTypeCode)
        {
        var actionRegistry= new ActionRegistry();
        var a = 1;
        var b = 2;
        //var actionResult= actionRegistry[this should come from your drop down].Invoke(a, b);
        var actionResult= actionRegistry[actionTypeCode].Invoke(a, b);

        return actionResult;
    }

Note that you should bind your dropdown list with your enum keys as value. I prefer to use an integer as my key because I can add more item later without changing my enum but it is not necessary.

| improve this answer | |
\$\endgroup\$
  • 3
    \$\begingroup\$ Simple and pragmatic solution :), You could also pass a string comparer to the dictionary's constructor to become a case insensitive dictionary: public ActionManager() : base(StringComparer.InvariantCultureIgnoreCase). How would you handle the case "action not available"? \$\endgroup\$ – JanDotNet Oct 10 '17 at 6:49
  • 1
    \$\begingroup\$ What about instead of using a String as an identifier, why not creating an Enum and then using it? This would put aside problems like aDD instead of Add or subtrat instead of Subtract. \$\endgroup\$ – auhmaan Oct 10 '17 at 9:12
  • 1
    \$\begingroup\$ One of the askers concerns was If I change the text in the drop down list, I need to change the function as well, and I don't see how that is addressed here. If the words in the text box change, this code still has to be revisited manually. \$\endgroup\$ – JPhi1618 Oct 10 '17 at 14:17
  • 1
    \$\begingroup\$ This is a good, extensible approach. Just don't call it a Manager (nor Helper, nor Utils). It's an ActionRegistry, or ArithmeticOperations. \$\endgroup\$ – Igor Soloydenko Oct 10 '17 at 15:18
  • 1
    \$\begingroup\$ @JPhi1618 you should read the key and your dropdown list key from one place. you can use an enum or constant or database for that by the way your calculate method won't ever change. \$\endgroup\$ – Pouya Samie Oct 10 '17 at 18:18
7
\$\begingroup\$

Is it worth changing? IMO: Code is best left as-is

It is worth employing some form of polymorphism for one if statement? You gotta be kidding me: for one if statement - to abstract it out? That would make it more complicated, would it not? In my opinion - probably not. cf: @t3chb0t who has a different view.

the code is small and very manageable. if it's already in place and working, and you have no reason to change it, then probably just leave it as is. there's nothing wrong with hard-coded code if it's working and will never change. but if changes start creeping in, then -- and only then consider refactoring. there's no point writing pure OOP for its own sake - perhaps that will make it harder to understand than it is now.

Or if you did want to make it easier to change (slightly), then you could just use an enum. If you change the enum value from “Add” to “Addition” you can ask Visual Studio or Resharper to do all the work for you – but if you wanted to add a new item or decide to get rid of one, then certainly you will be making changes in two places and the open closed principle will be thus violated. Also note how the use of the temporary variables are somewhat more cleaned up below:

internal class Program
{
    enum ListItem { Add, Subtract }        

    private int Calculate(ListItem action)
    {            
        if (action == ListItem.Add)
        {
            return 1234; // implement the calculation
        }
        else if(action == ListItem.Subtract)
        {
            return 1234; // implement the calculation
        }
        else
        {
            return 1234; // implement the calculation
        }
    }

}

You could go for a polymorphic solution - as an academic exercise?

You could implement the command design pattern and have a look at the MVVM design pattern (google them) - or other similar polymorphic solution. but you've already written the code!

@PouyaSamie has written a very elegant solution (and it's beautiful in its sophistication) @MartinBeam has a nice answer too. In conclusion my question to you as you read some of the polymorphic solutions - (which are all great - and with the utmost respect to those folks): does it make it easier to understand the code or harder?

| improve this answer | |
\$\endgroup\$
  • 7
    \$\begingroup\$ The code is not best as-is, it's horrible. Everything is hardcoded. You should always bother or otherwise you never learn ;-] \$\endgroup\$ – t3chb0t Oct 10 '17 at 7:47
  • \$\begingroup\$ @t3chb0t chrs appreciate the feedback/comment. \$\endgroup\$ – BKSpurgeon Oct 10 '17 at 8:09
  • 1
    \$\begingroup\$ @t3chb0t There are issues with the code (e.g. variable naming, weird use of class-level fields, repetition of magic strings). But for the actual approach, an if-else chain or switch statement seem fine to me. If you try to treat every problem as a toy version of a guessed-at much more complex problem, all you'll end up teaching yourself is how to overengineer yourself into a corner.. because usually, when your problem does become more complex, it's not in exactly the way you anticipated. Going with a simple, good design now then iterating on it later is an important skill. \$\endgroup\$ – Ben Aaronson Oct 10 '17 at 17:59
  • 2
    \$\begingroup\$ @BenAaronson of course, but the simplest design would be using a dictionary for it. An if such as this one is an absolute no-go. Maybe if you write a toy-app this is acceptable but there is no place for such code in any production code. Personally, I can't stand if else ;-] \$\endgroup\$ – t3chb0t Oct 10 '17 at 18:03
  • 1
    \$\begingroup\$ While I'm personally not that strict to if-else constructions, this particular question is specifically asking for a more flexible solution, quote: I do not wish to hard coded it to compare the action. The strategy pattern is a way to go here. \$\endgroup\$ – Igor Soloydenko Oct 10 '17 at 19:28
4
\$\begingroup\$

Based on @Pouya Samie's answer a slightly different solution:

  • ActionManager encapsulates a dictionary that holds the actions.
  • Note that the dictionary's keys are not case sensitive.
  • You can even extend the action manager's functionality (Remove actions from outside, get list of all available actions, ...)
  • [x] OCP: Just add / remove list of actions without modifying the ActionManager
  • [x] SRP: ActionManager holds and manages the relation between action's name and its logic

_

public class ActionManager
{
    private readonly Dictionary<string, Func<int, int, int>> myActions = 
        new Dictionary<string, Func<int, int, int>>(StringComparer.InvariantCultureIgnoreCase);

    public void Add(string key, Func<int, int, int> action) => myActions.Add(key, action);

    public int Calculat(string action, int value1, int value2)
    {
        Func<int, int, int> func;
        if (!myActions.TryGetValue(action, out func))
            throw new InvalidOperationException($"Action '{0}' does not exist.");       
        return func(value1, value2);
    }
}

Usage:

public class MyClass
{
    private readonly ActionManager myActionManager = new ActionManager();

    public MyClass()
    {
        myActionManager.Add("Add", (a, b) => a + b);
        myActionManager.Add("Subtract", (a, b) => a - b)
        // ...
    }

    public int X { get; set; }
    public int Y { get; set; }

    private int Calculate(string action) => myActionManager.Calculat(action, X, Y);
}
| improve this answer | |
\$\endgroup\$
2
\$\begingroup\$

I tend to avoid else if statements. If I’m just checking conditions and manipulating a value, then I’ll return in multiple if statements like this:

private int Calculate(string sAction)
{
    int iTotal = 0;

    if(sAction == "Add")
    {
        return X+Y;
    }

    if(sAction == "Subtract")
    {
       return X-Y;
    }     

    // No if statements entered.
    // Just return original value.

    return iTotal;
}

But, looking at your arguments, I’d say it’s a good candidate for the Strategy pattern. In fact, the Wikipedia entry on the Strategy pattern covers this very scenario! You could have a factory that returns the correct strategy, and then call that:

private int Calculate(string sAction)
{
    int iTotal = 0;

    CalculateActionFactory factory = new CalculateActionFactory;

    ICalculateAction action = factory.StrategyFor(sAction);

    return action.Execute(X, Y);
}

public class CalculateActionFactory
{
    public ICalculateAction StrategyFor(string Action)
    {
        switch (Action)
        {
            case "Add":
                return new AddAction;

            case "Minus":
                return new MinusAction;
        }

        throw new System.ArgumentException("Invalid action specified.");
    }
}

public interface ICalculateAction
{
    int Execute(int X, int Y);
}

public class AddAction : ICalculateAction
{
    public int Execute(int X, int Y)
    {
        return X+Y;
    }
}

public class MinusAction : ICalculateAction
{
    public int Execute(int X, int Y)
    {
        return X-Y;
    }
}

Disclaimer: I’m not a C# developer, so apologies if the syntax is off!

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ When using an abstraction for the actions, I would tend to add a "Name" property. Then you can replace the switch statement with something like:actions.FirstOrDefault(a => a.Name == name) ;) \$\endgroup\$ – JanDotNet Oct 10 '17 at 10:53
  • 2
    \$\begingroup\$ in my opinion, the formal strategy pattern as you wrote is just an old hack for the former lack of first order functions, and seems more or less obsolete now \$\endgroup\$ – Austin_Anderson Oct 10 '17 at 14:59
  • \$\begingroup\$ @Austin_Anderson: IMHO the strategy pattern is not obsolete: 1.) what if the strategy have more than one operation or other properties (e.g. additional description). 2.) If the algorithm is complex (e.g. consist of multiple functions and/or other object), then the concrete strategy class is a nice place to place that logic. Functional style may be succinct, but it does not always result in more readable and maintainable code. Therefore object oriented patterns should not necessarily be replaced by functional ones (IMO). \$\endgroup\$ – JanDotNet Oct 10 '17 at 16:11
  • \$\begingroup\$ @JanDotNet fair enough it still has it's place, but it's definitely overkill for this problem \$\endgroup\$ – Austin_Anderson Oct 10 '17 at 19:37
0
\$\begingroup\$

What about this?

public interface IAction
{
    int Calculate();

    string DisplayName { get; }
}

You can implement as many as you need, bind to the combobox and unit test it pretty good.

| improve this answer | |
\$\endgroup\$

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.