Doubts regarding Enum Strategy approach

For computing Income Tax, I tried to use Enum Strategy approach to make the logic more concise. But in the end I’m not satisfied mainly because I had to write different enums for income groups, limits and rates. This was because they are all constants of different nature. But due to this the code doesn’t looks compact and seems to lack encapsulation.

Is my worry genuine? Please let me know your views or a better approach.

Say, Income Tax groups and corresponding rates are as follows:

• Income between 0 – 500000 : 10% taxable
• Income between 5开发者_如何学JAVA00001 – 1000000 : 20% taxable
• Income between 1000001 – Infinite : 30% taxable

For example, Income Tax applicable on an income of 1200000 would be 30 % of 200000 (1200000 – 1000000) + 20% of 500000 (1000000 – 500000) + 10% of 500000 (500000 – 0) = 160000

Code:

package itcalc;

public class IncomeTaxCalculatorImpl {

    private enum IncomeGroup {

        ONE(Limit.GROUP_ONE_LIMIT) {

            @Override
            public double calculate(double income) {
                return income * Rate.GROUP_ONE_RATE.value / 100;
            }
        },
        TWO(Limit.GROUP_TWO_LIMIT) {

            @Override
            public double calculate(double income) {
                return ((income - Limit.GROUP_ONE_LIMIT.maximum)
                    * Rate.GROUP_TWO_RATE.value / 100)
                    + ONE.calculate(Limit.GROUP_ONE_LIMIT.maximum);
            }
        },
        THREE(Limit.GROUP_THREE_LIMIT) {

            @Override
            public double calculate(double income) {
                return ((income - Limit.GROUP_TWO_LIMIT.maximum)
                    * Rate.GROUP_THREE_RATE.value / 100)
                    + TWO.calculate(Limit.GROUP_TWO_LIMIT.maximum
                    - Limit.GROUP_ONE_LIMIT.maximum)
                    + ONE.calculate(Limit.GROUP_ONE_LIMIT.maximum);
            }
        };
        private final Limit limit;

        private enum Limit {

            GROUP_ONE_LIMIT(0, 500000),
            GROUP_TWO_LIMIT(500001, 1000000),
            GROUP_THREE_LIMIT(1000001, Double.MAX_VALUE);
            private final double minimum;
            private final double maximum;

            private Limit(double minimum, double maximum) {
                this.minimum = minimum;
                this.maximum = maximum;
            }
        }

        private enum Rate {

            GROUP_ONE_RATE(10), GROUP_TWO_RATE(20), GROUP_THREE_RATE(30);
            private final double value;

            private Rate(double value) {
                this.value = value;
            }
        }

        private IncomeGroup(Limit limit) {
            this.limit = limit;
        }

        abstract double calculate(double income);
    }

    public double calculate(double income) {
        for (IncomeGroup group : IncomeGroup.values()) {
            if (income >= group.limit.minimum
                && income <= group.limit.maximum) {
                return group.calculate(income);
            }
        }

        throw new IllegalArgumentException("Invalid Income Value");
    }
}

Are Limit and Rate enums being reused elsewhere? If not, you can get rid of them and modify IncomeGroup to have the limit and rate elements:

ONE(0,500000,10)

This way, you will have little less enums and the code will be little more compact

Enums are useful for making code more readable, but I imagine your code is not often going to refer to constants ONE, TWO or THREE in isolation, it's more likely to need to iterate through all the tax groups while performing a calculation.

The code would probably be clearer if you separate the tax group constants from the tax calculation algorithm. Using an array of objects, you can write:

public static final TaxGroup[] TAX_GROUPS = new TaxGroup[] {
  new TaxGroup(1000000, 0.3),
  new TaxGroup(500000, 0.2),
  new TaxGroup(0, 0.1)
};

You can then write a single algorithm to calculate the tax, rather than maintaining three separate copies of the arithmetic:

int untaxed = income;
int tax = 0;
for(TaxGroup taxGroup : TAX_GROUPS) {
    if(untaxed > taxGroup.lowerLimit) {
        tax += (untaxed - taxGroup.lowerLimit) * taxGroup.rate;
        untaxed = taxGroup.lowerLimit;
    }
}

You don't need the Limit or Rate classes. You can tell this because there is a one-to-one correspondance between the Rate, Limit and IncomeGroup - incorporate the values they represent into IncomeGroup, giving you three values for the group: two for the range, one for the percentage rate. You can do this even if you are using the values elsewhere - just make them public final fields of the Enums.

Incidentally if you are using doubles then the minimum for a range should be the maximum of the lower range, not the maximum plus one. Tax forms usually give these values because they round incomes to the nearest dollar. If you're not doing this you may get an income that falls into none of the ranges.

Even more incidentally, calculate() should probably be static, saving you the trouble of working out which income group you are in.

As I would expect for tax calculations, you will probably need to account for more and more facts over time. E.g. the ranges defining income groups might need to be loaded from a database since these could be change. Likewise the applicable rate for a given income group could change. Not a good idea to hard code these ranges into your code!!

You may want to look at the ideas behind a rules engine such as forward chaining. JBoss's Drools manual does a good job of sketching how you can use a rules engine (and the ideas transfer across most rules engine).

At any rate, I think the enum strategy pattern works for a simpler business cases (e.g. printing a given object such as org.joda.time.format.DateTimeFormatter) but not for more complex cases which may need to aggregate facts from multiple objects to calculate a result (e.g. calculations for payroll, taxes, reconciling books, etc.)