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

What are best practices for writing conditional statements?

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

Problem

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.

Solution


  • Extract repeated code into separate methods.



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



  • Don't forget to handle invalid input.



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



  • 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?

Code Snippets

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;

Context

StackExchange Code Review Q#29668, answer score: 6

Revisions (0)

No revisions yet.