Strategy Design Pattern in ASP.Net Core MVC Project

Question

I tried to make a calculator using the Strategy pattern in ASP.Net Core MVC (following an example from the Internet)

Please review my code and tell me what could be wrong and how to make it better?

My interface:

public interface ICalculator
 {
 double Calculate(double FirstNumber, double SecondNumber);
 }

I also have 4 classes (Addition, Multiplication, Division, Subtraction) I will attach only one of them because they are almost identical.

public class Addition : ICalculator
 {
 public double Calculate(double FirstNumber, double SecondNumber)
 {
 return FirstNumber + SecondNumber;
 }

Also here is my Context class.

 public class Context
 {
 private ICalculator calculator;
 public Context (ICalculator operation)
 {
 calculator = operation;
 }
 public double execute (double FirstNumber, double SecondNumber)
 {
 return calculator.Calculate(FirstNumber, SecondNumber);
 }
 }

Now here are my MVC elements. Here is my model and controller.

public class CalcModel 
 {
 public double FirstNumber { get; set; }
 public double SecondNumber { set; get; }
 public double Result { get; set; }
 public CalculationMethod calculationMethod { get; set; }
 public enum CalculationMethod
 {
 Addition = '+',
 Substraction = '-',
 Multiplication ='*',
 Division = '/'
 }
 }

Controller:

 public IActionResult IndexCalculator()
 {
 return View(new CalcModel());
 }
 [HttpPost]
 public async Task<IActionResult> IndexCalculator(CalcModel model)
 {
 
 ModelState.Clear();
 switch (model.calculationMethod)
 {
 case CalculationMethod.Addition:
 Context myAddition = new Context(new Addition());
 model.Result = myAddition.execute(model.FirstNumber, model.SecondNumber);
 break;
 case CalculationMethod.Division:
 Context myDivision = new Context(new Division());
 model.Result = myDivision.execute(model.FirstNumber, model.SecondNumber);
 break;
 case CalculationMethod.Multiplication:
 Context myMultiplication = new Context(new Multiplication());
 model.Result = myMultiplication.execute(model.FirstNumber, model.SecondNumber);
 break;
 case CalculationMethod.Substraction:
 Context mySubstraction = new Context(new Substraction());
 model.Result = mySubstraction.execute(model.FirstNumber, model.SecondNumber);
 break;
 }
 return View(model);
 }

Answer 1

Here are my observations:

ICalculator

• As far I as I understand this code's main purpose is to educate yourself. The whole strategy pattern for this extremely simple problem seems an overkill for me. You have written a lots of boilerplate code just to implement something really basic.
• Without knowing the concrete requirements double might be an overkill, with its 8 bytes.
  • My advice is to try to restrict the possible value range as much as possible, so in your case a float might be a suitable option as well
• FirstNumber: In C# we usually name the method's parameters with camelCasing rather than PascalCasing
• In the context of calculator it might make more sense to rename your parameters:
  • leftHandSide and rightHandSide
  • lhsOperand and rhsOperand
  • etc.

Addition

• I would advice to use expression bodied methods to make your implementations more concise

public float Calculate(float lhsOperand, float rhsOperand) => lhsOperand + rhsOperand;

• Your strategies have a lots of boiler plate code just to define the operator:

public float Calculate(float lhsOperand, float rhsOperand) => lhsOperand + rhsOperand;
public float Calculate(float lhsOperand, float rhsOperand) => lhsOperand - rhsOperand;
public float Calculate(float lhsOperand, float rhsOperand) => lhsOperand / rhsOperand;
public float Calculate(float lhsOperand, float rhsOperand) => lhsOperand * rhsOperand;

• If your problem scope would not be this extremely simple then your strategy should only contain the thing which differs and apply that in the context.

public float Execute(float lhsOperand, float rhsOperand)
{
 // Common logic
 ...
 // Different logic based on the chosen strategy
 float intermittentResult = calculator.ApplyOperator(lhsOperand, rhsOperand);
 // Common logic
 ...
 
 return finalResult;
}

Context

• calculator: This member should be marked as readonly to avoid unintentional replacement
• ICalculation operation: Please try to stick to the same terminology everywhere to make your code more readable. Using different terms interchangeably might confuse the code maintainer.
• execute: Please use PascalCasing for method names >> Execute

CalcModel

• This abbreviation is absolutely unnecessary and it breaks the consistency / coherence >> CalculationModel
• CalculationMethod: Here the terminology is again confusing. In math this is called operator. In the Context class you have referred to the interface implementation as operation. Please try to use consistent naming.
• Based on the provided code the values of the enum members do not matter. So, you could omit them without breaking anything.
• calculationMethod: In C# property names are usually using PascalCasing, please use consistent naming.

IndexCalculator

• 90% of all the branches share the same code
  • This could (and should) be fairly easily refactored:

ICalculator calculator = null;
switch (model.calculationMethod)
{
 case CalculationMethod.Addition:
 calculator = new Addition();
 break;
 case CalculationMethod.Division:
 calculator = new Division();
 break;
 case CalculationMethod.Multiplication:
 calculator = new Multiplication();
 break;
 case CalculationMethod.Substraction:
 calculator = new Substraction();
 break;
 default:
 throw new NotSupportedException($"The provided operator ('{model.calculationMethod}') is unknown.");
}
Context ctx = new Context(calculator);
model.Result = ctx.execute(model.FirstNumber, model.SecondNumber);

• As you can see the common code is extracted and the switch case focuses only on the differences
• I've also added a default branch to handle the unknown operator case
• Since C# 8 you can use switch expression which makes your branching more concise:

ICalculator calculator = model.calculationMethod switch
{
 CalculationMethod.Addition => new Addition(),
 CalculationMethod.Division => new Division(),
 CalculationMethod.Multiplication => new Multiplication(),
 CalculationMethod.Substraction => new Substraction(),
 _ => throw new NotSupportedException($"The provided operator ('{model.calculationMethod}') is unknown."),
};

• WarmingZ: Thank you so much for such a review! Yes, I used the strategy for educational purposes.

Answer 2

This is clearly a training exercise, and reviews on those are notable different because it makes no sense to address the overkill pattern for the simple implementation of mathematical operations. The implementations is simple specifically because the focus here is on the pattern.

Overall, pattern-wise your code is on the right path.

Naming-wise, I think you've missed the mark. Context seems to be the calculator here, as it performs the calculation. What I mean by that is that you could use a Context object repeatedly, to perform the same mathematical operation of different sets of inputs. In that sense, the Context performs each individual calculation (by passing it down to the specific operation that it was set to perform).
ICalculation would be better renamed to be IOperation, and Context should be Calculator. It makes more sense from a semantical point of view. My code assumes this name change has occurred.

One thing that does stick out to me, though, is the controller action. It should not contain the switch, because it violates SRP.

The purpose of a controller method is to map the incoming request to (and outgoing response from) your business logic. It should not contain your business logic itself, because this leads to difficult to read code.

A more elegant refactoring would be:

[HttpPost]
public async Task<IActionResult> IndexCalculator(CalcModel model)
{
 ModelState.Clear();
 var operation = GetOperation(model.calculationMethod);
 var calculator = new Calculator(operation);
 model.Result = calculator.Calculate(model.FirstNumber, model.SecondNumber);
 return View(model);
}

This method body is much clearer to read, from the perspective of the controller action. The controller doesn't care how you decide which operation to calculate with. It cares that you get an operation (decided by some other submethod), and that you then set the model's property based on said calculation.

Being vague about the specifics of which operation is being chosen here is actually beneficial to the readability of this controller action.

To achieve this GetOperation can be separated into a submethod of its own. For simplicity, I chose to use a private method in the controller here, but you could definitely abstract this further into a class of its own when the logic is complex enough to warrant it.

Generally speaking, this kind of logic belongs to the business layer, but it's so trivial in this case that creating an entire business layer for it is a bit overkill (and not the focus of the exercise either).

private IOperation GetOperation (CalculationMethod calcMethod)
 => calcMethod switch
 {
 CalculationMethod.Addition => new Addition(),
 CalculationMethod.Subtraction => new Subtraction(),
 CalculationMethod.Multiplication => new Multiplication(),
 CalculationMethod.Division => new Division(),
 _ => throw new NotImplementedException($"{nameof(GetOperation)} - CalculationMethod.{calcMethod} was not mapped to an operation!");
 };

I made use of C# 8's new switch pattern (and expression-bodied methods) here because it fits the use case. However, there were several possible ways of implementing this. You could've used the traditional switch, or a Dictionary<CalculationMethod, Func<IOperation> as a collection of factory methods, or even an OperationFactory altogether. But this is not the focus of the training exercise you're doing here, so I went with what was shortest and sweetest.