3
\$\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 parantheses.

The correct answer however would be 6.

So my code first splits the entire equation into a List<string>:

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

Then it checks if 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
2
\$\begingroup\$

Review

  • The calculator requires a very specific syntax and depends on whitespace delimiters. As a consumer it is really easy to make a mistake in using the calculator.
  • I would expect a basic calculator to include parentheses parsing as well as the basic operators +-/*.
  • You split the input in a series of string tokens List<string> equationList. String is hard to work with once you start extending the calculator. Invest in a custom class Token that has at least a Value (string) and a Type (Operator, Operand, Parenthesis, Whitespace, ..)
  • Method PunktVorStrich return the same list as provided in the parameters. It could be made void if you allow the provided list to get changed in the method body.
  • I would not perform the multiplication/division evaluation in the preprocessor. It should be done in a later phase.

Limitations

You have created a preprocessor function that transforms any input string into one that only uses addition and subtraction.

"1 + 1 * 3"

comes out as

"1 + 3"

Since the expression still remains to be evaluated, you'd have to go over the expression again, this time only dealing with the basic operators. I would not use a function like PunktVorStrich in the preprocessing phase, I would include it in the expression evaluator. This would gain performance, by only iterating the expression once.

By the way, how would you evaluate this expression? There is no consensus in math.

"2 / 3 * 4"

If you want to extend your calculator, you'll notice it would get harder to maintain the evaluator, since you have a fixed format <number><whitespace><operator>.. and evaluate left to right. As you noticed, a simple multiplication and division already are special cases to this left to right scanner. The following simple expressions can currently not be parsed.

"1+1"
"(1 + 1) + 1"

Expression Tree Evaluator

What you could do, is tokenize the input, build an expression tree and evaluate the tree. For instance "(1 + 1) + 1" could have a syntax tree:

//             add
//     add          1
//  1       1

You should read more about evaluating expressions using lexers and parsers. Try refactoring the simple evaluator with only +-/* to this structure.

\$\endgroup\$
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
-1
\$\begingroup\$

this is called Operators Precedence, you can work with this by placing "(" around your formulas.

Say you plan to do

(buy-Sell) * Sales Tax = Tax on Invoice

leave away the brackets and it would execute like this

buy - (Sell * Sales Tax) = BAD Tax on Invoice

You can "google" Operators Precedence to see the operators precedence applied for a given language as it applies to more than * and /

Have a look at this link that goes and explains a bit in C#

\$\endgroup\$

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.