HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpModerate

Calculator-like application

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
calculatorlikeapplication

Problem

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

Solution

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

Code Snippets

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

Context

StackExchange Code Review Q#5606, answer score: 12

Revisions (0)

No revisions yet.