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;
}
$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?
c# beginner
$endgroup$
add a comment |
$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?
c# beginner
$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
add a comment |
$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?
c# beginner
$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
c# beginner
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
add a comment |
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
add a comment |
5 Answers
5
active
oldest
votes
$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.
$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 aString
as an identifier, why not creating anEnum
and then using it? This would put aside problems likeaDD
instead ofAdd
orsubtrat
instead ofSubtract
.
$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 aManager
(norHelper
, norUtils
). It's anActionRegistry
, orArithmeticOperations
.
$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
|
show 5 more comments
$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?
$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. Anif
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 standif else
;-]
$endgroup$
– t3chb0t
Oct 10 '17 at 18:03
1
$begingroup$
While I'm personally not that strict toif-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
|
show 1 more comment
$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);
}
$endgroup$
add a comment |
$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!
$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
add a comment |
$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.
$endgroup$
add a comment |
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
});
}
});
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
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
$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.
$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 aString
as an identifier, why not creating anEnum
and then using it? This would put aside problems likeaDD
instead ofAdd
orsubtrat
instead ofSubtract
.
$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 aManager
(norHelper
, norUtils
). It's anActionRegistry
, orArithmeticOperations
.
$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
|
show 5 more comments
$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.
$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 aString
as an identifier, why not creating anEnum
and then using it? This would put aside problems likeaDD
instead ofAdd
orsubtrat
instead ofSubtract
.
$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 aManager
(norHelper
, norUtils
). It's anActionRegistry
, orArithmeticOperations
.
$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
|
show 5 more comments
$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.
$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.
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 aString
as an identifier, why not creating anEnum
and then using it? This would put aside problems likeaDD
instead ofAdd
orsubtrat
instead ofSubtract
.
$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 aManager
(norHelper
, norUtils
). It's anActionRegistry
, orArithmeticOperations
.
$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
|
show 5 more comments
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 aString
as an identifier, why not creating anEnum
and then using it? This would put aside problems likeaDD
instead ofAdd
orsubtrat
instead ofSubtract
.
$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 aManager
(norHelper
, norUtils
). It's anActionRegistry
, orArithmeticOperations
.
$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
|
show 5 more comments
$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?
$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. Anif
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 standif else
;-]
$endgroup$
– t3chb0t
Oct 10 '17 at 18:03
1
$begingroup$
While I'm personally not that strict toif-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
|
show 1 more comment
$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?
$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. Anif
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 standif else
;-]
$endgroup$
– t3chb0t
Oct 10 '17 at 18:03
1
$begingroup$
While I'm personally not that strict toif-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
|
show 1 more comment
$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?
$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?
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. Anif
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 standif else
;-]
$endgroup$
– t3chb0t
Oct 10 '17 at 18:03
1
$begingroup$
While I'm personally not that strict toif-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
|
show 1 more comment
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. Anif
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 standif else
;-]
$endgroup$
– t3chb0t
Oct 10 '17 at 18:03
1
$begingroup$
While I'm personally not that strict toif-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
|
show 1 more comment
$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);
}
$endgroup$
add a comment |
$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);
}
$endgroup$
add a comment |
$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);
}
$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);
}
answered Oct 10 '17 at 8:28
JanDotNetJanDotNet
7,0131339
7,0131339
add a comment |
add a comment |
$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!
$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
add a comment |
$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!
$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
add a comment |
$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!
$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!
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
add a comment |
$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
add a comment |
$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.
$endgroup$
add a comment |
$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.
$endgroup$
add a comment |
$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.
$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.
answered Oct 10 '17 at 7:01
ogomrubogomrub
1464
1464
add a comment |
add a comment |
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.
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
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
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
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
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