3
\$\begingroup\$

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 && number2 > 0)
        {
            double tempNumber = (number2 + Math.Abs(number1))/Math.Abs(number1);
            Math.Abs(percentageDifference = 100 - tempNumber);
        }

        if (number2 < 0 && number1 > 0)
        {
            double tempNumber = (number1 + Math.Abs(number2))/Math.Abs(number2);
            Math.Abs(percentageDifference = 100 - tempNumber);
        }
        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);
            }
        }


        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 culture
            outValue = CultureInfo.CurrentCulture.NumberFormat.NumberDecimalSeparator == ","
                       ? Convert.ToDouble(value, CultureInfo.InvariantCulture)
                       : Convert.ToDouble(value, new CultureInfo("pl-PL"));
        }
        catch (InvalidCastException)
        {
        }


        return outValue;
    }
\$\endgroup\$
  • \$\begingroup\$ Math.Abs(percentageDifference = 100 - tempNumber); This is uncommon usage and, while it works, I would avoid it. Use percentageDifference = Math.Abs(100 - tempNumber); instead \$\endgroup\$ – Kevin Aug 24 '15 at 15:32
  • \$\begingroup\$ Do you want an accurate calculation or do you just want to refactor these weird calcs? Consider that input values (-30,30) will yield a 98% difference from these calcs! Also wouldn't the difference between zero and a non-zero number be 100%? \$\endgroup\$ – Octopus Aug 24 '15 at 21:19
5
\$\begingroup\$

Some general observations first before I go into my refactored proposal:

  1. I would try and use a better parameter naming rather than temp1, temp2. To be honest I'm not sure if this is better but perhaps percentage1, percentage2 at least indicates the value is expected to be a percentage.

  2. Try and get spelling correct. CalcualtePercentageDifference should be CalculatePercentageDifference

  3. Consistent spacing. Sometimes you have spaces after your if statements sometimes you don't. I personally prefer spaces.

  4. Math.Abs returns a value. You aren't using that value so the calls to this method make no sense. Either remove it or use the value? For the purpose of this review, I'm going to assume you want to use the value.

  5. You have redundant brackets in some of your if statements. Consider removing these.

    if ((number1 >= number20))
    
  6. If a variable isn't required in a function, then I would suggest you remove it.

    private static double ParseDouble(object value)
    
  7. ParseDouble potentially returns NaN. I think you can account for that in code without letting general exceptions occur.

  8. I'm not sure why you need to swallow the InvalidCastException. In fact, is swallowing the exceptions at all what you are after? In general I try not to do this, but it can depend on the situation, so perhaps in this case it's ok.

Now for the code:

public string CalculatePercentageDifference(string percentage1, string percentage2)
{
    double number1 = ParseDouble(percentage1);
    double number2 = ParseDouble(percentage2);

    if (double.IsNaN(number1) || double.IsNaN(number2))
    {
        return "NaN";
    }

    var percentageDifference = CalculatePercentageDifference(number1, number2);
    return percentageDifference + " %";
}

Parse double without the additional parameter:

private static double ParseDouble(object value)
{
    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 culture
        try
        {
            outValue = CultureInfo.CurrentCulture.NumberFormat.NumberDecimalSeparator == ","
                       ? Convert.ToDouble(value, CultureInfo.InvariantCulture)
                       : Convert.ToDouble(value, new CultureInfo("pl-PL"));
        }
        catch (FormatException)
        {
            // If we still get here then there's something wrong
        }
    }
    catch (InvalidCastException)
    {
    }

    return outValue;
}

And the main refactored method:

public int CalculatePercentageDifference(double number1, double number2)
{
    if (number1.Equals(0) || number2.Equals(0))
    {
        return 0;
    }

    const double tolerance = 0.000000001;            
    // If numbers are the same then there is no difference
    if (Math.Abs(number1 - number2) < tolerance)
    {
        return 0;
    }

    Func<double, int> percentageDifference = number =>
    {
        var difference = Math.Abs(100 - number);
        return (int) Math.Round(difference);
    };

    Func<double, double, int> differenceHighLow = (numberHigh, numberLow) =>
    {
        var number = numberHigh * 100 / numberLow;

        return percentageDifference(number);
    };

    // Convert to positive numbers if they are both negative
    if (number1 < 0 && number2 < 0)
    {
        number1 = Math.Abs(number1);
        number2 = Math.Abs(number2);
    }

    // At least one of the numbers are positive at this stage
    if (number1 > 0 && number2 > 0)
    {
        return number1 > number2 ? 
            differenceHighLow(number2, number1) : 
            differenceHighLow(number1, number2);
    }

    // One number is a negative but the other is positive
    Func<double, double, int> postiveNegativeDifference = (postiveNumber, negativeNumber) =>
    {
        var tempNumber = (postiveNumber + Math.Abs(negativeNumber)) / Math.Abs(negativeNumber);
        return percentageDifference(tempNumber);
    };

    if (number1 < 0 && number2 > 0)
    {
        return postiveNegativeDifference(number2, number1);
    }

    return postiveNegativeDifference(number1, number2);
}

And of course a refactoring wouldn't be anywhere without some unit tests (of course, more would be recommended):

[TestMethod]
public void DifferenceBetweenZero_IsZero()
{
    // Arrange
    var model = new _101777_PercentageCalcManyIfs();

    // Act
    var difference = model.CalculatePercentageDifference("0.0", "0.0");

    // Assert
    Assert.AreEqual("0 %", difference);
}

[TestMethod]
public void DifferenceBetweenNonNumbers_IsNan()
{
    // Arrange
    var model = new _101777_PercentageCalcManyIfs();

    // Act
    var difference = model.CalculatePercentageDifference("a.0", "0.0");

    // Assert
    Assert.AreEqual("NaN", difference);
}

[TestMethod]
public void DifferenceBetweenTwoPostiveNumbers()
{
    // Arrange
    var model = new _101777_PercentageCalcManyIfs();

    // Act
    var difference = model.CalculatePercentageDifference("5.0", "10.0");

    // Assert
    Assert.AreEqual("50 %", difference);
}
| improve this answer | |
\$\endgroup\$
  • 3
    \$\begingroup\$ There is no block like: if (()number1 >= number20) \$\endgroup\$ – Quill Aug 24 '15 at 10:34
  • 2
    \$\begingroup\$ Consistent spacing. Sometimes you have spaces after your if statements sometimes you don't. I prefer personally spaces. I can't see any instances of extraneous lines after if statements. \$\endgroup\$ – Quill Aug 24 '15 at 11:08
5
\$\begingroup\$

Let the operators and the numbers have some space to breathe. Looking at

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 < 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);
        }
    }  

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.

| improve this answer | |
\$\endgroup\$
5
\$\begingroup\$

There's a much simpler way than many if-else loops:

if (number1 === 0 || number2 === 0) {
    return 0;
}
if (number1 > number2) {
    double tempNumber = number2;
    number2 = number1;
    number1 = tempNumber;
}
double difference = Math.Abs(100 - ((number1 / number2) * 100));
return Math.Round(difference) + " %";

By testing for the larger variable, you can escape the need for two 'which-is-larger' loops.


A few things to point out about your code:

  • valueName is unused.
  • catch (InvalidCastException): there's probably not a reason to swallow this, but either way.
  • string temp1, string temp2: they're not temporary values, don't name them like that.
  • number1*100/number2: add some whitespace and even brackets to clearly show which values affect each other, don't let BODMAS sort out your mess for you!
| improve this answer | |
\$\endgroup\$
  • 5
    \$\begingroup\$ Regarding your observation about the InvalidCastException, it falls under the Boneheaded exception category: "That argument is null, that typecast is bad, that index is out of range, you're trying to divide by zero – these are all problems that you could have prevented very easily in the first place, so prevent the mess in the first place rather than trying to clean it up." \$\endgroup\$ – dcastro Aug 24 '15 at 12:32
  • 3
    \$\begingroup\$ And the solution would be to change the type of the parameter value from object to string (since it'll always be a string anyway), and then using double.TryParse instead of Double.Convert. You won't need to catch FormatExceptions either. \$\endgroup\$ – dcastro Aug 24 '15 at 12:39

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.