What are best practices for writing conditional statements?

Question

I am doing some console test program, and I have some normal math operations. Example of part of my code:

if (flagMultiplyDivide.Equals(true))
{
    if (currentOperator.Equals('*'))
    {
        currentNumberDouble = multiplyNumbers(currentNumberString, temporaryNumberString);
        if (operatorOnHold.Equals('+'))
        {
            total += currentNumberDouble;
        }
        else if (operatorOnHold.Equals('-'))
        {
            total -= currentNumberDouble;
        }
        else
        {
            total = currentNumberDouble;
        }
        flagMultiplyDivide = false;
    }
    else if (currentOperator.Equals('/'))
    {
        currentNumberDouble = divideNumbers(temporaryNumberString, currentNumberString);
        if (operatorOnHold.Equals('+'))
        {
            total += currentNumberDouble;
        }
        else if (operatorOnHold.Equals('-'))
        {
            total -= currentNumberDouble;
        }
        else
        {
            total = currentNumberDouble;
        }
    }
}
else if (currentOperator.Equals('+'))
{
    if (!numberOnHold.Equals(""))
    {
        total += Convert.ToDouble(currentNumberString) + Convert.ToDouble(numberOnHold);
    }
    else
    {
        total += Convert.ToDouble(currentNumberString);
    }


}
else if (currentOperator.Equals('-'))
{
    if (!numberOnHold.Equals(""))
    {
        total -= Convert.ToDouble(currentNumberString) - Convert.ToDouble(numberOnHold);
    }
    else
    {
        total -= Convert.ToDouble(currentNumberString);
    }

}

return total;

Since it is logical program, I have a lot of conditional statements, and it is easy to lose in code. What is the best practice to code conditional statements in my case? How would you improve my part of code?

What am I doing wrong? I never have a problem to make a working code, but I really want to learn best practice, really. That's why I am here.

Answer 1

1. Extract repeated code into separate methods.
2. Use == to compare values, C# is not Java, most of the time, you don't need Equals(). And in the case of bools, you don't need to compare them at all.
3. Don't forget to handle invalid input.
4. It's probably better to use string.Empty instead of "", to make it clear that you didn't just forget to fill the right value in and you really meant the empty string.
5. Consider not using braces everywhere. It can make your code more readable.

(This code assumes all variables are fields; if that's not suitable to you, you will need to add some parameters and return values to the methods.)

void MultiplyOrDidide(bool multiply)
{
    if (multiply)
        currentNumberDouble = multiplyNumbers(currentNumberString, temporaryNumberString);
    else
        currentNumberDouble = divideNumbers(temporaryNumberString, currentNumberString);

    if (operatorOnHold == '+')
        total += currentNumberDouble;
    else if (operatorOnHold == '-')
        total -= currentNumberDouble;
    else
        total = currentNumberDouble;

    flagMultiplyDivide = false;        
}

void AddOrSubtract(bool add)
{
    double current = Convert.ToDouble(currentNumberString);
    double onHold = 0;
    if (numberOnHold != string.Empty)
        onHold = Convert.ToDouble(numberOnHold);

    if (add)
        total += current + onHold;
    else
        total -= current - onHold;
}

…

if (flagMultiplyDivide)
{
    if (currentOperator == '*')
        MultiplyOrDidide(multiply: true);
    else if (currentOperator == '/')
        MultiplyOrDidide(multiply: false);
    else
        throw new InvalidOperationException();
}
else if (currentOperator == '+')
{
    AddOrSubtract(add: true);
}
else if (currentOperator == '-')
{
    AddOrSubtract(add: false);
}
else
{
    throw new InvalidOperationException();
}

return total;

Also, the code around - (and +) seems suspicious to me. Shouldn't you check operatorOnHold there too?

Answer 2

I see two main issues with your code:

1. You are writing what looks like a simple expression evaluator. This is something what most will call reinventing the wheel (i.e. a waste of time). There are multiple libraries (both managed and unmanaged) which do just that. Flee beeing one such example (i used it successfully in the past).

2. Even if you do need to write something like that for some reason (studying?), i think your approach is wrong. Imho, you have one mess of a code because you are not separating parsing from actual evaluation. What you should do is you should first parse the expression into binary tree (represented by Expression class or your own binary tree implementation) getting rid of all the strings in your code. And the when you are done - you should evaluate this tree. There is an awesome article on CodeProject on this subject, which is most likely an overkill in your case, but still a good source of design ideas for your application.