patterncsharpMinor
What are best practices for writing conditional statements?
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:
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.
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 needEquals(). And in the case ofbools, you don't need to compare them at all.
- Don't forget to handle invalid input.
- It's probably better to use
string.Emptyinstead 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.