2

I need some help refactoring this if-statement. I thought of declaring the percentage as constants. I also thought to make a method that includes the code inside the if brackets. What else can i do?

if(totalReceiptsAmount >= getIncome() && totalReceiptsAmount <  0.20 * getIncome())
        setTaxIncrease(getBasicTax() + 0.05 * getBasicTax());
    if(totalReceiptsAmount >=  0.20 * getIncome() && totalReceiptsAmount <  0.40 * getIncome())
        setTaxIncrease(getBasicTax() - 0.05 * getBasicTax());
    if(totalReceiptsAmount >=  0.40 * getIncome() && totalReceiptsAmount <  0.60 * getIncome())
        setTaxIncrease(getBasicTax() - 0.10 * getBasicTax());
    if(totalReceiptsAmount >=  0.60 * getIncome())
        setTaxIncrease(getBasicTax() - 0.15 * getBasicTax());
6
  • Use else ifs to remove the double checks and factor out the function call, storing only the percentages inside the ifs. Commented May 3, 2015 at 19:27
  • well i did that at first, but our professor told us to refactor else if with only if Commented May 3, 2015 at 19:30
  • That feels wrong... Any reason for that? Commented May 3, 2015 at 19:31
  • 3
    This question is more appropriate for Code Review Commented May 3, 2015 at 19:34
  • 2
    The first if - how is totalReceiptsAmount going to be both greater than getIncome() and less than 0.2 * getIncome()? I'm assuming the income is not negative. Commented May 3, 2015 at 19:35

1 Answer 1

3

The main problem in your code is probably the duplication of code, meaning if you want to change e.g., the condition, you'd probably have to apply the same change in all four conditions. So you could try to factor out the common functionality, as you already suggested for the conditionals. So you could define a method

private boolean receiptsAmountIsBetweenFactorOfXAndYOfIncome(double x, double y){
    return totalReceiptsAmount >= x * getIncome() && totalReceiptsAmount < y  * getIncome();
}

and update your if-statements accordingly:

if(receiptsAmountIsBetweenFactorOfXAndYOfIncome(0, 0.2))
    setTaxIncrease(getBasicTax() + 0.05 * getBasicTax());
if(receiptsAmountIsBetweenFactorOfXAndYOfIncome(0.2, 0.4))
    setTaxIncrease(getBasicTax() - 0.05 * getBasicTax());
if(receiptsAmountIsBetweenFactorOfXAndYOfIncome(0.4, 0.6))
    setTaxIncrease(getBasicTax() - 0.10 * getBasicTax());
if(receiptsAmountIsBetweenFactorOfXAndYOfIncome(0.6, 1))
    setTaxIncrease(getBasicTax() - 0.15 * getBasicTax());

Now, there's still duplication in the body of the if statements. So you could introduce an other method:

private void increaseTaxByFactorOfX(double x){
    setTaxIncrease(getBasicTax() + x * getBasicTax());
}

and update the if-statements again:

if(receiptsAmountIsBetweenFactorOfXAndYOfIncome(0, 0.2))
    increaseTaxByFactorOfX(0.05);
if(receiptsAmountIsBetweenFactorOfXAndYOfIncome(0.2, 0.4))
    increaseTaxByFactorOfX(-0.05);
if(receiptsAmountIsBetweenFactorOfXAndYOfIncome(0.4, 0.6))
    increaseTaxByFactorOfX(-0.10);
if(receiptsAmountIsBetweenFactorOfXAndYOfIncome(0.6, 1))
    increaseTaxByFactorOfX(-0.15);

Now, if you want, you can detect a pattern in the used numericals, or simply hardcode the numericals in an array or a list and use a loop instead of multiple similar if-statements:

double[] factorOfIncome = {0, 0.2, 0.4, 0.6, 1};
double[] taxIncreaseFactor = {0.05, -0.05, -0.10, -0.15};

for(int i = 0; i<taxIncreaseFactor.length; i++)
    if(receiptsAmountIsBetweenFactorOfXAndYOfIncome(factorOfIncome[i], factorOfIncome[i+1]))
        increaseTaxByFactorOfX(taxIncreaseFactor[i]);

This last refactoring step gets completely rid of the duplication, but in my opinion makes the code a bit less understandable.

Edit: Note that I assumed that the first conditional should be

if(totalReceiptsAmount >= 0 * getIncome() && //... 

as it really looks like this is what you intended to write. If this isn't the case, then the first conditional would need to be treated separately.

Sign up to request clarification or add additional context in comments.

Comments

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.