2
\$\begingroup\$

I'm working on a calculator for educational purposes and was asked to include a rule that multiplies/divides before it adds/subtracts.

Let's assume I have the following equation

2 + 2 * 2

Windows built-in calculator would return 8, since it just goes down the line without paranthesis.

The correct answer however would be 6.

So my code first splits the entire equation into a List

string equation = "2 + 2 * 2"
List<string> equationList = equation.Split(' ').ToList();
//Result:
//2
//+
//2
//*
//2

Then it checks if the equationList contains an asterisk or slash

if (equationList.Contains("*")) equationList = PunktVorStrich("*", equationList);
if (equationList.Contains("/")) equationList = PunktVorStrich("/", equationList);

This automatically calls the following function PunktVorStrich

private List<string> PunktVorStrich(string symbol, List<string> equationList)
{
    while (equationList.Contains(symbol))
    {//loops in case there are multiple instances of the given symbol
        int position = equationList.IndexOf(symbol); //position of the symbol
        int before = int.Parse(equationList.ElementAt(position - 1)); //position of the number before the symbol
        int after = int.Parse(equationList.ElementAt(position + 1)); //position of the number after the symbol
        equationList.RemoveAt(position - 1);
        equationList.RemoveAt(position - 1);
        equationList.RemoveAt(position - 1);//Remove all 3 elements
        //finally calculate and "replace" the equation-part with the result
        if(symbol == "*")
        {
            equationList.Insert(position - 1, (before * after).ToString());
        }else if(symbol == "/")
        {
            equationList.Insert(position - 1, (before / after).ToString());
        }
    }
    return equationList;
    //equationList =
    //2
    //+
    //4
}

And finally the program loops through the remaining equation:

bool number = true; //This ensures that the equation is split between operators and values
int total = 0; //Sum of the current equation
string lastOperator = String.Empty; //Stores the last operator

foreach (string term in equationList)
{
    if (number)
    {
        if (lastOperator == "") total = int.Parse(term);
        if (lastOperator == "+") total = total + int.Parse(term);
        if (lastOperator == "-") total = total - int.Parse(term);
    }
    number= !number;
    lastOperator = term;
}

Finally I can return the equation with

MessageBox.Show(total.ToString());

I'm however not exactly happy with this solution, since I personally think that took a few steps too much to achieve my goal. Is there anything I can do better?

\$\endgroup\$
  • \$\begingroup\$ Instead of remove and insert I would just create a new list. It breaks if there are not spaces. Is parsing down a string a requirement? \$\endgroup\$ – paparazzo Oct 19 '17 at 16:16
  • \$\begingroup\$ The user can not input equations directly, the calculator is a windows form that adds the spaces automatically after you pressed one of the operator buttons. It's not a requirement to parse the string down, I just thought it would make the whole calculating process easier if i break down the input and loop through it. \$\endgroup\$ – GummiBoat Oct 19 '17 at 18:11
  • \$\begingroup\$ Create a class with operator and value and just pass the list. \$\endgroup\$ – paparazzo Oct 19 '17 at 18:15
  • \$\begingroup\$ What would the benefit of using a seperate class for this purpose be? \$\endgroup\$ – GummiBoat Oct 19 '17 at 18:18
  • \$\begingroup\$ So you don't need to parse a string. \$\endgroup\$ – paparazzo Oct 19 '17 at 18:19
0
\$\begingroup\$

I would start by simplifying the calls to PunktVorStrich. You currently test whether the symbol is there at all, but then your while loop does that in PunktVorStrich also. No need to do it in both places. Also, since PunktVorStrich modifies the parameter object (and List is passed by reference), no need to resassign to equationList when it returns.

PunktVorStrich("*", equationList);

But more importantly, your PunktVorStrich can do all of your logic for you. It currently compresses your expression by evaluating a sub-expression and replacing the sub-expression with the result. This could be done from start to finish in your whole expression.

I would create a constant string array of symbols, in the order you want to process them. Then set up a loop which will pass each one into PunktVorStrich. You will need to modify PunktVorStrich to handle plus and minus as well, but that won't be hard.

const string[] SYMBOLS = new string[] {"*", "/", "+", "-"};
for (int i = 0; i < SYMBOLS.Length; i++)
    PunktVorStrich(SYMBOLS[i], equationList);

A few more small things:

  1. Change the if block in PunktVorStrich to be a switch statement. It's more efficient for this.
  2. Inside the switch, only do the unique part of the process (the actual mathematical evaluation). Then do the Insert after the switch.
  3. You may want to reconsider using int to store the result, since you allow division.
\$\endgroup\$
  • \$\begingroup\$ I also support @Paparazzi's idea. \$\endgroup\$ – wildbagel Oct 19 '17 at 18:45
  • \$\begingroup\$ Those are really neat suggestions, thank you very much. Took me a while to understand everything, I'll try this out tomorrow. \$\endgroup\$ – GummiBoat Oct 19 '17 at 18:56
0
\$\begingroup\$

If you don't need to parse a string then don't

public enum MathOp { add, subtract, multiply, divide)
public class MathItem 
{
    public int Value { get; set; }
    public MathOp MathOp { get; set; }
}

Then pass a List<MathItem> and no string parsing is required

\$\endgroup\$
  • \$\begingroup\$ thank you for the suggestion, i'll try to implement it tomorrow :) \$\endgroup\$ – GummiBoat Oct 19 '17 at 18:26

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.