Add or subtract two numbers depending on dropdown selection





.everyoneloves__top-leaderboard:empty,.everyoneloves__mid-leaderboard:empty,.everyoneloves__bot-mid-leaderboard:empty{ margin-bottom:0;
}







8












$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?










share|improve this question











$endgroup$








  • 7




    $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


















8












$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?










share|improve this question











$endgroup$








  • 7




    $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














8












8








8


1



$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?










share|improve this question











$endgroup$




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?







c# beginner






share|improve this question















share|improve this question













share|improve this question




share|improve this question








edited Oct 10 '17 at 16:09









200_success

131k17157422




131k17157422










asked Oct 10 '17 at 5:44









Derick LooDerick Loo

14613




14613








  • 7




    $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














  • 7




    $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








7




7




$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




$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










5 Answers
5






active

oldest

votes


















9












$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.






share|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?






share|improve this answer











$endgroup$









  • 6




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





share|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!






    share|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.






    share|improve this answer









    $endgroup$














      Your Answer






      StackExchange.ifUsing("editor", function () {
      StackExchange.using("externalEditor", function () {
      StackExchange.using("snippets", function () {
      StackExchange.snippets.init();
      });
      });
      }, "code-snippets");

      StackExchange.ready(function() {
      var channelOptions = {
      tags: "".split(" "),
      id: "196"
      };
      initTagRenderer("".split(" "), "".split(" "), channelOptions);

      StackExchange.using("externalEditor", function() {
      // Have to fire editor after snippets, if snippets enabled
      if (StackExchange.settings.snippets.snippetsEnabled) {
      StackExchange.using("snippets", function() {
      createEditor();
      });
      }
      else {
      createEditor();
      }
      });

      function createEditor() {
      StackExchange.prepareEditor({
      heartbeatType: 'answer',
      autoActivateHeartbeat: false,
      convertImagesToLinks: false,
      noModals: true,
      showLowRepImageUploadWarning: true,
      reputationToPostImages: null,
      bindNavPrevention: true,
      postfix: "",
      imageUploader: {
      brandingHtml: "Powered by u003ca class="icon-imgur-white" href="https://imgur.com/"u003eu003c/au003e",
      contentPolicyHtml: "User contributions licensed under u003ca href="https://creativecommons.org/licenses/by-sa/3.0/"u003ecc by-sa 3.0 with attribution requiredu003c/au003e u003ca href="https://stackoverflow.com/legal/content-policy"u003e(content policy)u003c/au003e",
      allowUrls: true
      },
      onDemand: true,
      discardSelector: ".discard-answer"
      ,immediatelyShowMarkdownHelp:true
      });


      }
      });














      draft saved

      draft discarded


















      StackExchange.ready(
      function () {
      StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f177586%2fadd-or-subtract-two-numbers-depending-on-dropdown-selection%23new-answer', 'question_page');
      }
      );

      Post as a guest















      Required, but never shown

























      5 Answers
      5






      active

      oldest

      votes








      5 Answers
      5






      active

      oldest

      votes









      active

      oldest

      votes






      active

      oldest

      votes









      9












      $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.






      share|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


















      9












      $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.






      share|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
















      9












      9








      9





      $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.






      share|improve this answer











      $endgroup$



      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.







      share|improve this answer














      share|improve this answer



      share|improve this answer








      edited Oct 10 '17 at 19:17

























      answered Oct 10 '17 at 6:38









      Pouya SamiePouya Samie

      22126




      22126








      • 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
















      • 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










      3




      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




      $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




      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




      $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




      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




      $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




      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




      $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




      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






      $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?






      share|improve this answer











      $endgroup$









      • 6




        $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
















      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?






      share|improve this answer











      $endgroup$









      • 6




        $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














      7












      7








      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?






      share|improve this answer











      $endgroup$



      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?







      share|improve this answer














      share|improve this answer



      share|improve this answer








      edited 20 mins ago

























      answered Oct 10 '17 at 6:14









      BKSpurgeonBKSpurgeon

      96129




      96129








      • 6




        $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














      • 6




        $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








      6




      6




      $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$
      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




      $begingroup$
      @t3chb0t chrs appreciate the feedback/comment.
      $endgroup$
      – BKSpurgeon
      Oct 10 '17 at 8:09




      1




      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




      $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




      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






      $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




      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




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





      share|improve this answer









      $endgroup$


















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





        share|improve this answer









        $endgroup$
















          4












          4








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





          share|improve this answer









          $endgroup$



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






          share|improve this answer












          share|improve this answer



          share|improve this answer










          answered Oct 10 '17 at 8:28









          JanDotNetJanDotNet

          7,0131339




          7,0131339























              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!






              share|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
















              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!






              share|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














              2












              2








              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!






              share|improve this answer









              $endgroup$



              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!







              share|improve this answer












              share|improve this answer



              share|improve this answer










              answered Oct 10 '17 at 10:22









              Martin BeanMartin Bean

              1879




              1879












              • $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


















              • $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
















              $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




              $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




              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$
              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$
              @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




              $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.






              share|improve this answer









              $endgroup$


















                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.






                share|improve this answer









                $endgroup$
















                  0












                  0








                  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.






                  share|improve this answer









                  $endgroup$



                  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.







                  share|improve this answer












                  share|improve this answer



                  share|improve this answer










                  answered Oct 10 '17 at 7:01









                  ogomrubogomrub

                  1464




                  1464






























                      draft saved

                      draft discarded




















































                      Thanks for contributing an answer to Code Review Stack Exchange!


                      • Please be sure to answer the question. Provide details and share your research!

                      But avoid



                      • Asking for help, clarification, or responding to other answers.

                      • Making statements based on opinion; back them up with references or personal experience.


                      Use MathJax to format equations. MathJax reference.


                      To learn more, see our tips on writing great answers.




                      draft saved


                      draft discarded














                      StackExchange.ready(
                      function () {
                      StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f177586%2fadd-or-subtract-two-numbers-depending-on-dropdown-selection%23new-answer', 'question_page');
                      }
                      );

                      Post as a guest















                      Required, but never shown





















































                      Required, but never shown














                      Required, but never shown












                      Required, but never shown







                      Required, but never shown

































                      Required, but never shown














                      Required, but never shown












                      Required, but never shown







                      Required, but never shown







                      Popular posts from this blog

                      Сан-Квентин

                      Алькесар

                      Josef Freinademetz