Calculator-like application

Question

I'm attempting to implement a calculator-like application in C#. But the way I've currently implemented it seems a little messy and inefficient. It works, but it's just I'm pretty sure it can be achieved in a more elegant solution.

I looked to see if I could factor out any repeating code, such as the switch statement - but without having numerous parameters in the method, I don't think it'll make things much better.

Here's the main portion of the code:

private double Calculate()
{
 double answer = 0.0;
 foreach (double number in this.numbers)
 {
 int index = this.numbers.IndexOf(number);
 if (index != this.numbers.Count() - 1)
 {
 if (answer != 0.0)
 {
 switch ((int)operators[index])
 {
 case (int)Operator.Plus:
 answer = (double)(answer + numbers[index + 1]);
 break;
 case (int)Operator.Minus:
 answer = (double)answer - numbers[index + 1];
 break;
 case (int)Operator.Divide:
 answer = (double)answer / numbers[index + 1];
 break;
 case (int)Operator.Multiply:
 answer = (double)answer * numbers[index + 1];
 break;
 }
 }
 else
 {
 switch ((int)operators[index])
 {
 case (int)Operator.Plus:
 answer += (double)(number + numbers[index + 1]);
 break;
 case (int)Operator.Minus:
 answer += (double)number - numbers[index + 1];
 break;
 case (int)Operator.Divide:
 answer += (double)number / numbers[index + 1];
 break;
 case (int)Operator.Multiply:
 answer += (double)number * numbers[index + 1];
 break;
 }
 }
 } 
 }
 return answer;
}

Just for reference, Operators is an ENUM that's defined as:

private enum Operator
{
 Plus = 1,
 Minus = 2,
 Multiply = 3,
 Divide = 4,
}

and the two lists:

private List<double> numbers;
private List<Operator> operators;

Any ideas on how I could improve this?

Answer 1

Your first big problem is going to be fighting your design. Multiple case statements like that are asking to be removed either due to polymorphism, or stuck deep in the bowls of some sort of AbstractFactory, and only referred to once (sorry, I've just been going the Clean Code book).
In this case, making this setup polymorphic is going to be your friend... Otherwise, you're going to have an interesting time later, trying to add, say modulous (%).

To start with, define an Operation interface. Something like this:

public interface Operation {
 public double Operate(double argA, double argB);
}

You then need to implement each operation seperately:

public struct Division : Operation {
 // For obvious reasons, the proper argument names should be specified
 public double Operate(double quotient, double divisor) {
 return quotient / divisor;
 }
}

This will allow you to add any number of future operations quite simply (including some really complicated stuff) - so long as it only takes 2 arguments... (note - I'm attempting to retain the general concept of your design; there are other possibilities here). And replace the List<Operator> with a List<Operation>.

Your calculate method then becomes, simply:

private double Calculate () {
 double answer = 0.0;
 for(int i = 0; i < numbers.Count(), i < operators.Count(), i++) {
 // Of course, this doesn't check for divide-by-zero...
 answer = operators[i].Operate(answer, numbers[i]); 
 }
 return answer;
 }

Your current method had the following issues -

1. Listing multiple copies of the same number, but with different operations, would re-use only one of the operations (probably the first in the list).
2. You aren't sufficiently checking for array out-of-bounds errors - if numbers is longer than operators, you'll run out of operators (it's debatable if ignoring numbers on the end is better, as I do...).
3. Your if (answer == 0.0) statement really encapsulates the exact same logic. You just need to look at it a little different.

Answer 2

Well, first of all there is a bug in the code. You are getting the index of a number by looking for it in the list, but if the number occured earlier in the list you will get the wrong index. You should use the index in the loop instead of using a foreach.

You are using the Count() extension method to get the number of items in the list, but you should use the Count property instead.

You are casting all enum values to integers, but you can just use the enum values directly.

You are casting a lot of values to double, but they are already double values.

You are using the value of answer in the code where you have determined that it's always zero.

You are comparing the answer value to zero to determine if it's the first iteration, but that could occur later in the loop too.

Instead of handling the first iteration differently, you can just put the first number in the answer variable and start the loop from 1.

The code suddenly got a lot simpler. :)

private double Calculate() {
 double answer = numbers[0];
 for (int index = 1; index < numbers.Count; index++) {
 switch (operators[index - 1]) {
 case Operator.Plus:
 answer += numbers[index];
 break;
 case Operator.Minus:
 answer -= numbers[index];
 break;
 case Operator.Divide:
 answer /= numbers[index];
 break;
 case Operator.Multiply:
 answer *= numbers[index];
 break;
 }
 }
 return answer;
}

• John Davis: Wow, that really is an improvement. Thanks for the advice!

Answer 3

How do people feel about replacing enums with classes and moving the logic into Funcs?

public class Operation<T>
{
 public string Name { get; set; }
 public Func<T, T, T> Calculation { get; set; }
}
private List<double> numbers;
private List<Operation<double>> operations = new List<Operation<double>>
{
 new Operation<double> {Name = "Add", Calculation = (x,y) => x + y},
 new Operation<double> {Name = "Subtract", Calculation = (x,y) => x - y},
 new Operation<double> {Name = "Multiply", Calculation = (x,y) => x * y},
 new Operation<double> {Name = "Divide", Calculation = (x,y) => x / y}
 //,... Add new ones as you like here
};
public double Calculate<T>()
{
 var answers = numbers[0];
 for (int index = 0; index < operations.Count; index++)
 {
 var operation = operations[index];
 operation.Calculation(answers, numbers[index]);
 }
 return answers;
}

Answer 4

X-Zero has some good suggestions. Depending on the intended complexity of your program, creating a polymorphic design as X-Zero suggested is definitely a good way to go - and it makes future code much easier. I would just add a few comments on those code suggestions, though:

• Especially if this code may be used as some form of public API, it may be a good idea to name the interface and implementations correctly. For example, Operation is an interface, so following standard naming conventions it should be changed to IOperation. Similarly, it might be worth naming the implementations as SomethingOperation. For example; Division would become DivisionOperation. This is not 100% necessary, but makes it easier to identify what the class is an implementation of, without looking into the code.

• One other point I would make, is that in the for loop, the first operation is executed using the default initial value of zero. If that first operation is a multiplication, this method will result in erroneous results. To resolve this, I would refer to the answer given by Guffa:

  You are comparing the answer value to zero to determine if it's the first iteration, but that could occur later in the loop too.

  Instead of handling the first iteration differently, you can just put the first number in the answer variable and start the loop from 1.

Note: The point made on the initial value of zero was referring to the answer given by X-Zero, not the code in the question.

• Guffa: Eventhough the initial value is zero, it's not used as one of the operands for the first operator, instead the code does answer + numbers[0] * numbers[1], so the result would still be correct.
• Samuel Slade: Are you referring to your own answer, or the one given by X-Zero?
• Guffa: Neither. I am referring to the original code, as I am commenting on your point that having an initial value of zero gives an erroneous result if the first operation is a multiplication.
• Samuel Slade: I was referring to the answer given by X-Zero, not the question. I have modified my answer to note this.
• Guffa: Ok, then it makes a valid point. I'm sorry that I misunderstood you.

Answer 5

You should be able to use the conditional operator to get it down to 1 switch statement. I haven't done C# in a long time so I'm not sure if this will compile.

bool answer_zero = (bool)(answer != zero);
switch ((int)operators[index])
{
 case (int)Operator.Plus:
 answer = (double)((answer_zero ? answer : number) + numbers[index + 1]);
 break;
 case (int)Operator.Minus:
 answer = (double)((answer_zero ? answer : number) - numbers[index + 1]);
 break;
 case (int)Operator.Divide:
 answer = (double)((answer_zero ? answer : number) / numbers[index + 1]);
 break;
 case (int)Operator.Multiply:
 answer = (double)((answer_zero ? answer : number) * numbers[index + 1]);
 break;
}