Parser for an arithmetic expression

Question

I am a student, and I am doing websites as freelancer. Lately I have started learning design patterns, refactoring, and test-driven development so that I can improve the maintainability, performance, and readability of my code.

I did some refactoring with method extracting in this case, but I got feedback that my code is trash.

Here is the main method. The whole Program class is on GitHub.

/* main functionality
 * 
 * parameter: expression - accepts string parameter and parses him, so it can calculate
 */
static double ParseCalculationString(string expression)
{

    // currentNumberString will save current number
    string currentNumberString = "";
    // temporaryNumberString will be used for multiply/divide operations
    string temporaryNumberString = "";

    // we use this to access previous operator for calculating between numbers
    char previousOperator = ' ';

    // we use this for current operator that we get from from expression[i]
    char currentOperator = ' ';

    /* this is used for sum and subtract operations, we save operator and number here
     so we can finish multiply and divide operations first, then we use number on hold
     and operator on hold to add or deduct to total result. */
    string numberOnHold = "";
    char operatorOnHold = ' ';

    /* total is used for saving total result, and currentNumberDouble for saving the number
     * in the middle of multiply/divide operations */
    double total = 0.0;
    double currentNumberDouble = 0.0;

    /* We use this two flags for marking the operation type, so we know what to calculate first */
    bool flagMultiplyDivide = false;
    bool flagSumSubtract = false;


    int i;



    for (i = 0; i < expression.Length; i++)
    {

        // checking if character is number, if yes we build number from the string until the character is symbol
        if (char.IsNumber(expression[i]))
        {
            currentNumberString = currentNumberString + expression[i];

        }
        // I only wanted to limit to 4 symbols in this case, so I checked if current input character is * or / or - or +
        else if (ValidateOperator(expression[i]))
        {
            currentOperator = expression[i];

            // we jump into this condition if operator between two numbers is * or /
            if (flagMultiplyDivide)
            {
                // if we multiply
                if (previousOperator.Equals('*'))
                {
                    currentNumberDouble = MultiplyNumbers(currentNumberString, temporaryNumberString);
                }

                // if we divide
                else if (previousOperator.Equals('/'))
                {
                    currentNumberDouble = DivideNumbers(temporaryNumberString, currentNumberString);
                }

                /* we have to use this condition if there is no more * or / operators after this number, therefore we can add our
                 multiply/divide result to total result*/
                if (operatorOnHold.Equals('-') && (!currentOperator.Equals('*') && !currentOperator.Equals('/')))
                {
                    total -= currentNumberDouble;
                    currentNumberDouble = 0.0;
                }


                else if (operatorOnHold.Equals('+') && (!currentOperator.Equals('*') && !currentOperator.Equals('/')))
                {
                    total += currentNumberDouble;
                    currentNumberDouble = 0.0;
                }

                currentNumberString = "";
                flagMultiplyDivide = false;
            }

            // we jump into this condition if operator between two numbers is + or -
            else if (flagSumSubtract)
            {

                // if current operator is not * or / we can safely sum or subtract to total result(order of operations rule)
                if (!currentOperator.Equals('*') && !currentOperator.Equals('/'))
                {
                    if (operatorOnHold.Equals('+'))
                    {
                        total = ApplyNumberOnHold(total, numberOnHold, currentNumberString, true);
                    }
                    else if (operatorOnHold.Equals('-'))
                    {
                        total = ApplyNumberOnHold(total, numberOnHold, currentNumberString, false);
                    }

                    numberOnHold = "";
                    currentNumberString = "";
                    flagSumSubtract = false;
                }
            }


            /* checking the current operators and setting the variables */

            // multiply and divide operations
            if (currentOperator.Equals('*') || currentOperator.Equals('/'))
            {
                // in case we have multiple consecutive multiply/divide operations 5*5*5*5
                if (!currentNumberDouble.Equals(0.0))
                {
                    currentNumberString = Convert.ToString(currentNumberDouble);
                }

                /* in case there is number and - or + after the number we can add it directly to total result
                   example for this: 5-5*5, which means that even if current operator is to multiply, we can safely
                   add or deduct our 5 to total result */
                if (!numberOnHold.Equals(""))
                {
                    total += Convert.ToDouble(numberOnHold);
                    numberOnHold = "";
                }

                temporaryNumberString = currentNumberString;
                flagMultiplyDivide = true;
                flagSumSubtract = false;

            }

            // sum and subtract operations
            else if (currentOperator.Equals('+') || currentOperator.Equals('-'))
            {
                // used when checking, if previous operation was multiply/divide, so he can add result of that operation to total result
                if (!currentNumberDouble.Equals(0.0))
                {
                    if (operatorOnHold.Equals('-'))
                        total -= currentNumberDouble;
                    else
                        total += currentNumberDouble;

                    currentNumberDouble = 0.0;
                }


                // if we have consecutive add/subtract operations we directly add them to total result
                if (!numberOnHold.Equals(""))
                {
                    if (operatorOnHold.Equals('-'))
                    {
                        total -= Convert.ToDouble(numberOnHold);
                    }
                    else
                    {
                        total += Convert.ToDouble(numberOnHold);
                    }
                }

                numberOnHold = currentNumberString;
                operatorOnHold = currentOperator;
                flagSumSubtract = true;
                flagMultiplyDivide = false;
            }

            currentNumberString = "";
            previousOperator = currentOperator;
        }
    }

    // if it is last character number, we have to check it after loop ends, because we can't loop no more
    if (flagMultiplyDivide)
    {
        if (previousOperator.Equals('*'))
        {
            currentNumberDouble = MultiplyNumbers(currentNumberString, temporaryNumberString);
            total = ApplyOperatorOnHold(operatorOnHold, total, currentNumberDouble);
        }
        else if (previousOperator.Equals('/'))
        {
            currentNumberDouble = DivideNumbers(temporaryNumberString, currentNumberString);
            total = ApplyOperatorOnHold(operatorOnHold, total, currentNumberDouble);
        }
    }
    else if (previousOperator.Equals('+'))
    {
        total = ApplyNumberOnHold(total, numberOnHold, currentNumberString, true);
    }
    else if (previousOperator.Equals('-'))
    {
        total = ApplyNumberOnHold(total, numberOnHold, currentNumberString, false);
    }
    return total;
}

Answer 1

In general comments should be used to say why you are doing something, not what you are doing. You should strive to have variable names that describe what they contain.

Don't be scared to create sub-functions. If you feel like you need to explain what a block of code is doing, there is a chance that block can be turned into a separate function. Now you can give the new function a descriptive name that will help explain what is being done. This will also help people from getting lost in the number of nested if blocks.

Define constants for your operators, possible make them an enum since they form a logical grouping. It will also make it easier to find places where you use the constants. Otherwise you have to do a text search for + and will end up finding places where that is used in the code and not as a parsing operator. Another benefit of defining constants is to avoid magic numbers. This is not something you are doing here, but is related to the same idea. It is a good practice to get into for values that contain a special meaning.

Answer 2

Comments As per one of the previous answers, I'm a big fan of commenting only what the code cannot say, not simply what it does not say.

With this in mind, I'd say most of your comments could come out and be replaced with either nothing (when the intent is obvious) or with an encapsulation into a method.

Code structure It feels very functional/linear at the moment - I feel that you could gain something here by encapsulation into a 'Calculator' class - you would find the overall flow of this method would be delegated over to that class and it would become responsible for it's own data.

Random stuff The main method body in ParseCalculationString makes this class difficult to trace through (I suspect I could debug through it to validate it far more easily) which adds fuel to the 'use smaller methods, break tasks out whenever possible explaining exactly what that method is doing'. Variable naming - it's not always clear on the intent of your variables - 'numberOnHold' is one of those where I'm clearer once I've read the comment. I like your use of guard clauses on your inputs up front, nicely done

Only real issue in all of this feels like 'ParseCalculationString' (which although a complex task feels like it could be broken down as highlighted either into a 'Calculator' class or into sub methods to make that overall method flow better.

Hope that helps.

Answer 3

Good use of descriptive variable and function names.  I'd like to see the index 'i' tell me more about what it is indexing.

Good comments on the intention of each little chunk.  I'd almost like to see some of the chunks broken out into little functions themselves and named appropriately, but the way you have it is good too, for now.

A bit too much comments on the reason for each variable at the beginning.  It should be crystal clear in the variable name and perhaps a bit of comment explanation on the intent when the variable is actually used.

FWIW, if it WORKS, I'd add it to production as is.  It's a good start and shows discipline, but the real test is when things start breaking or people want changes.  Can you maintain that methodical, clear variables naming and clear intent commenting under pressure and under what are inevitably strange requests from the end user?

PS

Another factor to keep in mind is how to identify where your code is brittle. Where are you going to get null reference exception, where are you going to get index out of range, numerical overflow, where are your floating point comparisons going to fail because of floating point non determinism. These things come with experience, but you can get a head start by taking a conscious deliberate approach to finding frail and error prone lines.