Solving Ayende's Tax Woes
Oren Eini (a.k.a. Ayende Rahien) is going through the joy that is trying to find good developers. The other day he posted some sample code that an interviewee had turned in to a coding challenge. The results weren't pretty!
The problem involves calculating a persons taxes based on a set of tax brackets.
It makes a great interview question because the problem is short enough to get done in an hour or so, but tricky enough to require some thought. Here are the specs as posted by Ayende:
I am a huge proponent of unit testing, and more specifically Test Driven Development. Since we have a nice set of acceptance tests, it is pretty easy to get those into code.
I started with the simplest of tests I could think of.
Which led me to create this simple implementation.
Alright, so not very useful. But the tests pass!
Now we need to be able to expand on this a bit and add a tax bracket in the mix. A tax bracket has some lower-bound, and a rate. We can write another pretty simple test for that.
(Note: The use of named parameters is to aid in the design process. Since I wrote the code before the method existed, the IDE can infer the parameter names and automatically generate a signature for me. Also, it helps to clarify what is going on in the test.)
We Add just enough code to get the tests passing, which is certainly more useful than the previous iteration, but far from complete. Some might object to my introducing a TaxBracket type into the mix so early, but writing just enough code doesn't preclude one from writing clean code. The extra type helps to tidy things up, and makes future refactoring work easier with little extra work.
This rev is also incomplete since it doesn't take the lower-bound into account. So... we write another test case to cover that scenario.
A minor change to the code, and now we can effectively handle a single flat tax, with an optional poverty line.
However, what we want is a system with multiple tax brackets. Now we get to the testing scenario that was previously laid out for us. We can easily translate those tables into a test.
It was obvious that we needed some sort of collection to hold the TaxBrackets, and that we would need to loop over them to calculate the effective tax. The decision to reverse the loop, greatly simplifies the processing.
Essentially we start with the highest amount of money that has a tax rate, and chop off that amount from the total amount remaining, taxing each chunk as we go and adding it to the total.
Finally we need to address a massive assumption with the current implementation. Mainly, that the tax brackets are in ascending order based on their lower bound. That is a pretty big assumption, and one that could lead to nasty bugs in the future.
For our last test, we mix up the ordering of the tax brackets and make sure everything still works.
Clearly this fails miserably so with a little adjustment to our method, we can ensure the ordering is correct before we start processing. The added benefit is that the code is now even more expressive than the explicit for loop from the previous iteration.
Now, there are some clear omissions here, like checking for negative values, but I think it accomplished the spirit of the question.
As I stated previously I haven't read any of the other solutions, so this may be an exact duplicate. Also, there may be much more elegant solutions to this problem, but as usual TDD has lead to something that is neat, clean, and well tested :)
The problem involves calculating a persons taxes based on a set of tax brackets.
It makes a great interview question because the problem is short enough to get done in an hour or so, but tricky enough to require some thought. Here are the specs as posted by Ayende:
Israel's Tax Brackets:
Tax Rate Up to 5,070 10% 5,071 up to 8,660 14% 8,661 up to 14,070 23% 14,071 up to 21,240 30% 21,241 up to 40,230 33% Higher than 40,230 45%
Acceptance Tests:
Having not yet read the other comments or solutions, I thought it would be fun to take a crack at writing my own solution to the problem.
- 5,000 –> 500
- 5,800 –> 609.2
- 9,000 –> 1087.8
- 15,000 –> 2532.9
- 50,000 –> 15,068.1
I am a huge proponent of unit testing, and more specifically Test Driven Development. Since we have a nice set of acceptance tests, it is pretty easy to get those into code.
I started with the simplest of tests I could think of.
[TestMethod]
public void ShouldCalculateTotalTaxesToBeZero() {
var effectiveTax = new EffectiveTax();
var result = effectiveTax.CalculateTaxes(0);
Assert.AreEqual(0, result);
}
Which led me to create this simple implementation.
public class EffectiveTax
{
public Decimal CalculateTaxes(Decimal grossIncome)
{
return 0;
}
}
Alright, so not very useful. But the tests pass!
Now we need to be able to expand on this a bit and add a tax bracket in the mix. A tax bracket has some lower-bound, and a rate. We can write another pretty simple test for that.
[TestMethod]
public void ShouldCalculateTaxesUsingSingleTaxBracket() {
var effectiveTax = new EffectiveTax();
effectiveTax.AddTaxBracket(lowerBound: 0, taxRate: 0.1M);
Assert.AreEqual(0, effectiveTax.CalculateTaxes(0));
Assert.AreEqual(100, effectiveTax.CalculateTaxes(1000));
Assert.AreEqual(1000, effectiveTax.CalculateTaxes(10000));
}
(Note: The use of named parameters is to aid in the design process. Since I wrote the code before the method existed, the IDE can infer the parameter names and automatically generate a signature for me. Also, it helps to clarify what is going on in the test.)
We Add just enough code to get the tests passing, which is certainly more useful than the previous iteration, but far from complete. Some might object to my introducing a TaxBracket type into the mix so early, but writing just enough code doesn't preclude one from writing clean code. The extra type helps to tidy things up, and makes future refactoring work easier with little extra work.
public class EffectiveTax {
private TaxBracket _taxBracket;
public Decimal CalculateTaxes(Decimal grossIncome) {
return grossIncome * _taxBracket.TaxRate;
}
public void AddTaxBracket(Decimal lowerBound, Decimal taxRate) {
_taxBracket = new TaxBracket {
LowerBound = lowerBound,
TaxRate = taxRate
};
}
private class TaxBracket {
internal Decimal LowerBound { get; set; }
internal Decimal TaxRate { get; set; }
}
}
This rev is also incomplete since it doesn't take the lower-bound into account. So... we write another test case to cover that scenario.
[TestMethod]
public void ShouldCalculateOnlyTaxableIncome() {
var effectiveTax = new EffectiveTax();
effectiveTax.AddTaxBracket(lowerBound: 5000, taxRate: 0.1M);
Assert.AreEqual(0, effectiveTax.CalculateTaxes(0));
Assert.AreEqual(0, effectiveTax.CalculateTaxes(4999));
Assert.AreEqual(0, effectiveTax.CalculateTaxes(5000));
Assert.AreEqual(10, effectiveTax.CalculateTaxes(5100));
}
A minor change to the code, and now we can effectively handle a single flat tax, with an optional poverty line.
public Decimal CalculateTaxes(Decimal grossIncome) {
var effectiveTax = 0.0M;
if (grossIncome > _taxBracket.LowerBound) {
var taxableIncome = grossIncome - _taxBracket.LowerBound;
effectiveTax = taxableIncome * _taxBracket.TaxRate;
}
return effectiveTax;
}
However, what we want is a system with multiple tax brackets. Now we get to the testing scenario that was previously laid out for us. We can easily translate those tables into a test.
[TestMethod]
public void ShouldCalculateTaxUsingIsraelsTaxBrackets() {
var effectiveTax = new EffectiveTax();
effectiveTax.AddTaxBracket(lowerBound: 0, taxRate: 0.1M);
effectiveTax.AddTaxBracket(lowerBound: 5070, taxRate: 0.14M);
effectiveTax.AddTaxBracket(lowerBound: 8660, taxRate: 0.23M);
effectiveTax.AddTaxBracket(lowerBound: 14070, taxRate: 0.30M);
effectiveTax.AddTaxBracket(lowerBound: 21240, taxRate: 0.33M);
effectiveTax.AddTaxBracket(lowerBound: 40230, taxRate: 0.45M);
Assert.AreEqual(0, effectiveTax.CalculateTaxes(0));
Assert.AreEqual(500, effectiveTax.CalculateTaxes(5000));
Assert.AreEqual(609.2M, effectiveTax.CalculateTaxes(5800));
Assert.AreEqual(1087.8M, effectiveTax.CalculateTaxes(9000));
Assert.AreEqual(2532.9M, effectiveTax.CalculateTaxes(15000));
Assert.AreEqual(15068.1M, effectiveTax.CalculateTaxes(50000));
}
It was obvious that we needed some sort of collection to hold the TaxBrackets, and that we would need to loop over them to calculate the effective tax. The decision to reverse the loop, greatly simplifies the processing.
Essentially we start with the highest amount of money that has a tax rate, and chop off that amount from the total amount remaining, taxing each chunk as we go and adding it to the total.
public class EffectiveTax {
private readonly List _taxBrackets = new List();
public Decimal CalculateTaxes(Decimal grossIncome) {
var effectiveTax = 0.0M;
var incomeLeftToTax = grossIncome;
for (var i = (_taxBrackets.Count - 1); i >= 0; i--) {
var taxBracket = _taxBrackets[i];
if (incomeLeftToTax > taxBracket.LowerBound) {
var taxableIncome = incomeLeftToTax - taxBracket.LowerBound;
incomeLeftToTax -= taxableIncome;
effectiveTax += taxableIncome * taxBracket.TaxRate;
}
}
return effectiveTax;
}
public void AddTaxBracket(Decimal lowerBound, Decimal taxRate) {
_taxBrackets.Add(new TaxBracket {
LowerBound = lowerBound,
TaxRate = taxRate
});
}
private class TaxBracket {
internal Decimal LowerBound { get; set; }
internal Decimal TaxRate { get; set; }
}
}
Finally we need to address a massive assumption with the current implementation. Mainly, that the tax brackets are in ascending order based on their lower bound. That is a pretty big assumption, and one that could lead to nasty bugs in the future.
For our last test, we mix up the ordering of the tax brackets and make sure everything still works.
[TestMethod]
public void ShouldCorrectlyOrderTaxBrackets() {
var effectiveTax = new EffectiveTax();
effectiveTax.AddTaxBracket(lowerBound: 40230, taxRate: 0.45M);
effectiveTax.AddTaxBracket(lowerBound: 14070, taxRate: 0.30M);
effectiveTax.AddTaxBracket(lowerBound: 0, taxRate: 0.1M);
effectiveTax.AddTaxBracket(lowerBound: 8660, taxRate: 0.23M);
effectiveTax.AddTaxBracket(lowerBound: 21240, taxRate: 0.33M);
effectiveTax.AddTaxBracket(lowerBound: 5070, taxRate: 0.14M);
Assert.AreEqual(0, effectiveTax.CalculateTaxes(0));
Assert.AreEqual(500, effectiveTax.CalculateTaxes(5000));
Assert.AreEqual(609.2M, effectiveTax.CalculateTaxes(5800));
Assert.AreEqual(1087.8M, effectiveTax.CalculateTaxes(9000));
Assert.AreEqual(2532.9M, effectiveTax.CalculateTaxes(15000));
Assert.AreEqual(15068.1M, effectiveTax.CalculateTaxes(50000));
}
Clearly this fails miserably so with a little adjustment to our method, we can ensure the ordering is correct before we start processing. The added benefit is that the code is now even more expressive than the explicit for loop from the previous iteration.
public Decimal CalculateTaxes(Decimal grossIncome) {
var effectiveTax = 0.0M;
var incomeLeftToTax = grossIncome;
var orderedBrackets = _taxBrackets.OrderByDescending(tb => tb.LowerBound);
foreach (var taxBracket in orderedBrackets) {
if (incomeLeftToTax > taxBracket.LowerBound) {
var taxableIncome = incomeLeftToTax - taxBracket.LowerBound;
incomeLeftToTax -= taxableIncome;
effectiveTax += taxableIncome * taxBracket.TaxRate;
}
}
return effectiveTax;
}
Now, there are some clear omissions here, like checking for negative values, but I think it accomplished the spirit of the question.
As I stated previously I haven't read any of the other solutions, so this may be an exact duplicate. Also, there may be much more elegant solutions to this problem, but as usual TDD has lead to something that is neat, clean, and well tested :)