5
\$\begingroup\$

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.

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

\$\endgroup\$
  • \$\begingroup\$ Always putting braces around if body reduces the probability of a bug, or am I brainwashed? You may have found a "local maximum", but I would ask a question - is there a better overall approach? Evaluating arithmetic expressions seems so short and easy in LISP. \$\endgroup\$ – Leonid Aug 13 '13 at 2:55
  • 1
    \$\begingroup\$ 5. In my company, for example, one of the code styling rules forbids you from not using braces after if statements except for cases when it is followed by throw or return (ironically for exactly the same reason - it improves readbility). And i agree with this policy. So, i guess, its a matter of taste. \$\endgroup\$ – Nikita B Aug 13 '13 at 4:50
  • \$\begingroup\$ @Nik I really don't think adding unnecessary braces improves readability. It does make it harder to write certain kind of bugs, but I think those are not common enough to justify the decrease in readability. But that's certainly debatable, which is why I said “consider” and not “do it”. \$\endgroup\$ – svick Aug 13 '13 at 8:01
  • \$\begingroup\$ @Leonid I think that probability is too low to justify the decrease in readability. Especially if there are not complete beginners in C-like languages on your team and you use good IDE that automatically formats your code. \$\endgroup\$ – svick Aug 13 '13 at 8:04
  • 1
    \$\begingroup\$ +1. Answers the question; and does error trapping!. Svick is right about braces causing significant clutter in this example. Alternatively we could just keep stuff on the same line: if (add) { total += current + onHold; }. It is, in the final analysis, a judgement call. \$\endgroup\$ – radarbob Aug 13 '13 at 15:40
5
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ I mustn't use the library, I had to write my own thing. This second example(ANTLR) you pasted me, is really overkill to understand for my brains at the moment. But it is cool thing to know. As I said I am kind of still student, and I am not that experienced in code design. I deliver working application, but I still have a lot to learn in terms of design. I found out this website that i really like and will learn from it: sourcemaking.com \$\endgroup\$ – Amel Salibasic Aug 15 '13 at 12:30

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.