patterncsharpMinor
Percentage calculation
Viewed 0 times
calculationpercentagestackoverflow
Problem
What would be the best way to refactor following methods?
```
private string CalcualtePercentageDifference(string temp1, string temp2)
{
double number1 = ParseDouble(temp1);
double number2 = ParseDouble(temp2);
double percentageDifference = 0;
if (number1 == 0 || number2 == 0)
{
return "0 %";
}
if ((number1 >= number2) && number1 > 0 && number2 > 0)
{
double tempNumber = number2*100/number1;
Math.Abs(percentageDifference = 100 - tempNumber);
}
if ((number2 >= number1) && number1 > 0 && number2 > 0)
{
double tempNumber = number1*100/number2;
Math.Abs(percentageDifference = 100 - tempNumber);
}
if (number1 0)
{
double tempNumber = (number2 + Math.Abs(number1))/Math.Abs(number1);
Math.Abs(percentageDifference = 100 - tempNumber);
}
if (number2 0)
{
double tempNumber = (number1 + Math.Abs(number2))/Math.Abs(number2);
Math.Abs(percentageDifference = 100 - tempNumber);
}
if (number1 = number2))
{
double tempNumber = number2 * 100 / number1;
Math.Abs(percentageDifference = 100 - tempNumber);
}
if ((number2 >= number1))
{
double tempNumber = number1 * 100 / number2;
Math.Abs(percentageDifference = 100 - tempNumber);
}
}
return Math.Round(percentageDifference) + " %";
}
private static double ParseDouble(object value, string valueName = "")
{
double outValue = double.NaN;
try
{
outValue = Convert.ToDouble(value);
}
catch (FormatException e)
{
//We will get here when value contains NumberDecimalSeparator different from this in current culture
//Convert now using suitable
```
private string CalcualtePercentageDifference(string temp1, string temp2)
{
double number1 = ParseDouble(temp1);
double number2 = ParseDouble(temp2);
double percentageDifference = 0;
if (number1 == 0 || number2 == 0)
{
return "0 %";
}
if ((number1 >= number2) && number1 > 0 && number2 > 0)
{
double tempNumber = number2*100/number1;
Math.Abs(percentageDifference = 100 - tempNumber);
}
if ((number2 >= number1) && number1 > 0 && number2 > 0)
{
double tempNumber = number1*100/number2;
Math.Abs(percentageDifference = 100 - tempNumber);
}
if (number1 0)
{
double tempNumber = (number2 + Math.Abs(number1))/Math.Abs(number1);
Math.Abs(percentageDifference = 100 - tempNumber);
}
if (number2 0)
{
double tempNumber = (number1 + Math.Abs(number2))/Math.Abs(number2);
Math.Abs(percentageDifference = 100 - tempNumber);
}
if (number1 = number2))
{
double tempNumber = number2 * 100 / number1;
Math.Abs(percentageDifference = 100 - tempNumber);
}
if ((number2 >= number1))
{
double tempNumber = number1 * 100 / number2;
Math.Abs(percentageDifference = 100 - tempNumber);
}
}
return Math.Round(percentageDifference) + " %";
}
private static double ParseDouble(object value, string valueName = "")
{
double outValue = double.NaN;
try
{
outValue = Convert.ToDouble(value);
}
catch (FormatException e)
{
//We will get here when value contains NumberDecimalSeparator different from this in current culture
//Convert now using suitable
Solution
Let the operators and the numbers have some space to breathe. Looking at
it is hard to see that the variable is called
The
can be simplified by removing the check for
The same is true for
In this construct you are calculating the
If you would declare
which would be more readable.
double tempNumber = number2*100/number1;it is hard to see that the variable is called
number2. So by adding some horizontal space will increase the readability like so double tempNumber = number2 * 100 / number1;The
if condition here if ((number2 >= number1) && number1 > 0 && number2 > 0)can be simplified by removing the check for
number2 > 0. If number2 is greater than or equal to number1 and number1 is greater than 0 then for sure number2 is also greater than 0. The same is true for
if ((number1 >= number2) && number1 > 0 && number2 > 0)if (number1 = number2))
{
double tempNumber = number2 * 100 / number1;
Math.Abs(percentageDifference = 100 - tempNumber);
}
if ((number2 >= number1))
{
double tempNumber = number1 * 100 / number2;
Math.Abs(percentageDifference = 100 - tempNumber);
}
}In this construct you are calculating the
percentageDifference two times if number1 and number2 are both < 0 and are the same. Using an else if would skip this. If you would declare
tempNumber, which by the way is named sub optimal, instead of percentageDifference outside of the if's you could remove a lot of code duplication by calling Math.Abs(percentageDifference = 100 - tempNumber); at the end of thr method. In addition why do you use it like this and not like percentageDifference = Math.Abs(100 - tempNumber);which would be more readable.
Code Snippets
double tempNumber = number2*100/number1;double tempNumber = number2 * 100 / number1;if ((number2 >= number1) && number1 > 0 && number2 > 0)if ((number1 >= number2) && number1 > 0 && number2 > 0)if (number1 < 0 && number2 < 0)
{
number1 = Math.Abs(number1);
number2 = Math.Abs(number2);
if ((number1 >= number2))
{
double tempNumber = number2 * 100 / number1;
Math.Abs(percentageDifference = 100 - tempNumber);
}
if ((number2 >= number1))
{
double tempNumber = number1 * 100 / number2;
Math.Abs(percentageDifference = 100 - tempNumber);
}
}Context
StackExchange Code Review Q#101777, answer score: 5
Revisions (0)
No revisions yet.