Add or subtract two numbers depending on dropdown selection

Question

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?

Answer 1

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.

Answer 2

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

It is worth employing some form of polymorphism for one if statement? 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?

Summary: avoid polymorphism until you really need to have it. simple is best, and an if statement isn't too complicated for professional programmers, and is much simpler than polymorphic approaches. my two cents worth.

Answer 3

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);
}

Answer 4

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!

Answer 5

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.