Name Your Problems
Sometimes when developing code, I run into problems. Ok, a lot of the times I run into problems, but there's a certain kind of problem I run into pretty often. The problem is that I want a thing that doesn't exist. I am writing code, and it is just getting gnarlier and more tangled around a pile of logic. At some point, I hit a wall with it. It is too complicated to really reason about, and I don't want to make things any worse. I've found a trick to solving these problems, though. I take those problems and give them a name. Once I do that, I can get a handle on them, manipulate them, or even sequester them off into a corner. In almost every case, this drastically reduces the complexity back down to a reasonable level, and I often get the side benefit of offering extensibility I didn't know I needed.
To demonstrate, let's say we are working on an e-commerce site. The company is based in Alabama, and sells to customers in all 50 states in the US. We have a method that computes the total price of the invoice that the customer gets charged.
public double calculateTotal(final Invoice invoice) {
double total = invoice.subtotal();
total += this.shippingService.calculateCost(invoice.shippingAddress());
return total;
}
It is a simple bit of code that gets the job done. The problem is that many of the items we sell are subject to sales tax in their destinations, and we aren't calculating that in. This should be pretty straightforward, though. Let's start with adding sales tax for our home state, Alabama. Alabama charges a flat 4% sales tax on all items we sell.
public double calculateTotal(final Invoice invoice) {
double total = invoice.subtotal();
if (invoice.shippingAddress().state().equals("AL")) {
total *= 1.04;
}
total += this.shippingService.calculateCost(invoice.shippingAddress());
return total;
}
That was easy. It made the code a little uglier, but not too bad. There are five states, Alaska, Delaware, Montana, New Hampshire, and Oregon, that don't charge sales tax. For them, we don't have to make any changes, meaning we are done! That's six states down, fourty-four to go. Let's do North Carolina next. They charge a flat 4.25% on all items we sell.
public double calculateTotal(final Invoice invoice) {
double total = invoice.subtotal();
if (invoice.shippingAddress().state().equals("AL")) {
total += (total + 0.04);
} else if (invoice.shippingAddress().state().equals("NC")) {
total += (total * 0.0425);
}
total += this.shippingService.calculateCost(invoice.shippingAddress());
return total;
}
Ok, I think something is happening here. Each state we add is probably going to be a new condition on on this if-else chain. Since we are switching on the state value, let's switch this up to a case statement to make it a little cleaner.
public double calculateTotal(final Invoice invoice) {
double total = invoice.subtotal();
switch (invoice.shippingAddress().state()) {
case "AL":
total += (total * 0.04);
break;
case "NC":
total += (total * 0.0425);
break;
default:
// No sales tax
break;
}
total += this.shippingService.calculateCost(invoice.shippingAddress());
return total;
}
Now it is a little clearer how to add a new state, and gives us a place to see that certain states don't collect income tax. Let's add a state where we have a lot of customers: Pennsylvania. Pennsylvania charges a 6% sales tax, but groceries and clothing are exempt. Some of the items we sell qualify as groceries and clothing, so we don't want to charge sales tax on those items.
public double calculateTotal(final Invoice invoice) {
double total = invoice.subtotal();
switch (invoice.shippingAddress().state()) {
case "AL":
total += (total * 0.04);
break;
case "NC":
total += (total * 0.0425);
break;
case "PA":
double taxable = 0.0;
for (Item item : invoice.items()) {
if (item.category() != Category.GROCERY &&
item.category() != Category.CLOTHING) {
taxable += item.price();
}
}
total += (taxable * 0.06);
break;
default:
// No sales tax
break;
}
total += this.shippingService.calculateCost(invoice.shippingAddress());
return total;
}
That got ugly real quick. This does what we want, but this method is really starting to sprawl. If we keep going down the path we are going, this method is going to end up being bloated with all of this different tax logic. It will get hard to maintain. It is already getting hard to test. For each state, we need to add cases for each different condition within the tax calculation. To test the final value, we also need to calculate in the shipping cost by hand to get our expected values. Let's give all this tax calculation a name by pulling it out to a private method.
public double calculateTotal(final Invoice invoice) {
double total = invoice.subtotal();
total += calculateTax(invoice, total);
total += this.shippingService.calculateCost(invoice.shippingAddress());
return total;
}
private static double calculateTax(final Invoice invoice, final double total) {
switch (invoice.shippingAddress().state()) {
case "AL":
return total * 0.04;
case "NC":
return total * 0.0425;
case "PA":
double taxable = 0.0;
for (Item item : invoice.items()) {
if (item.category() != Category.GROCERY &&
item.category() != Category.CLOTHING) {
taxable += item.price();
}
}
return (taxable * 0.06);
default:
// No sales tax
return 0.0;
}
}
Things are a little better now. The tax calculation is no longer in the calculateTotal method. It is sequestered off into its own area. This still isn't great though. We haven't solved the testing problem, because we can only test this through the same public calculateTotal method. This is a necessary intermediate step, though. Only once it is in its own method can we cleanly move it to a new class,
public double calculateTotal(final Invoice invoice) {
double total = invoice.subtotal();
total += TaxCalculator.calculateTax(invoice, total);
total += this.shippingService.calculateCost(invoice.shippingAddress());
return total;
}
class TaxCalculator {
public static double calculateTax(final Invoice invoice, final double total) {
switch (invoice.shippingAddress().state()) {
case "AL":
return total * 0.04;
case "NC":
return total * 0.0425;
case "PA":
double taxable = 0.0;
for (Item item : invoice.items()) {
if (item.category() != Category.GROCERY &&
item.category() != Category.CLOTHING) {
taxable += item.price();
}
}
return (taxable * 0.06);
default:
// No sales tax
return 0.0;
}
}
}
This is much better. We've named the thing that was our problem. It is a tax calculator. This class is now isolated. It is easier to test because we can feed it inputs and get outputs directly. And we can change it in whatever way we feel like. Let's split out each state into its own private method to make them even more self-contained.
public double calculateTotal(final Invoice invoice) {
double total = invoice.subtotal();
total += TaxCalculator.calculateTax(invoice, total);
total += this.shippingService.calculateCost(invoice.shippingAddress());
return total;
}
class TaxCalculator {
public static double calculateTax(final Invoice invoice, final double total) {
switch (invoice.shippingAddress().state()) {
case "AL":
return calculateALTax(invoice, total);
case "NC":
return calculateNCTax(invoice, total);
case "PA":
return calculatePATax(invoice, total);
default:
// No sales tax
return 0.0;
}
}
private static double calculateALTax(final Invoice invoice, final double total) {
return total * 0.04;
}
private static double caclulateNCTax(final Invoice invoice, final double total) {
return total * 0.0425;
}
private static double calculatePATax(final Invoice invoice, final double total) {
double taxable = 0.0;
for (Item item : invoice.items()) {
if (item.category() != Category.GROCERY &&
item.category() != Category.CLOTHING) {
taxable += item.price();
}
}
return (taxable * 0.06);
}
}
Interesting. Now that we've given names to all the individual pieces, we can see a pattern. Each state, internally, can calculate sales tax in their own way, but the interface is consistent between each state. Maybe a tax calculator is not a utility. Maybe it is a polymorphic type. Let's convert it to an interface.
public double calculateTotal(final Invoice invoice) {
double total = invoice.subtotal();
total += TaxCalculator.calculateTax(invoice, total);
total += this.shippingService.calculateCost(invoice.shippingAddress());
return total;
}
interface TaxCalculator {
public static double calculateTax(final Invoice invoice, final double total) {
switch (invoice.shippingAddress().state()) {
case "AL":
return new ALTaxCalculator().calculateTax(invoice, total);
case "NC":
return new NCTaxCalculator().calculateTax(invoice, total);
case "PA":
return new PATaxCalculator().calculateTax(invoice, total);
default:
// No sales tax
return 0.0;
}
}
double calculate(Invoice invoice, double total);
}
class ALTaxCalculator implements TaxCalculator {
@Override
public double calculate(final Invoice invoice, final double total) {
return total * 0.04;
}
}
class NCTaxCalculator implements TaxCalculator {
@Override
public double caclulate(final Invoice invoice, final double total) {
return total * 0.0425;
}
}
class PATaxCalculator implements TaxCalculator {
@Override
public double calculate(final Invoice invoice, final double total) {
double taxable = 0.0;
for (Item item : invoice.items()) {
if (item.category() != Category.GROCERY &&
item.category() != Category.CLOTHING) {
taxable += item.price();
}
}
return (taxable * 0.06);
}
}
Each state is now its own class. They can be tested and modified individually. We've gotten to a great place. The one thing that is still a little ugly is the static call to the calculateTax method. Each case in there is doing the same thing: creating an instance of the tax calculator for the state, then calling calculate on it. We can call the calculate method in the calculateTotal method if we have something that gives us back the right tax calculator. Let's move that calculateTax method over to a new class.
public double calculateTotal(final Invoice invoice) {
double total = invoice.subtotal();
total += TaxCalculators.calculateTax(invoice, total);
total += this.shippingService.calculateCost(invoice.shippingAddress());
return total;
}
class TaxCalculators {
public static double calculateTax(final Invoice invoice, final double total) {
switch (invoice.shippingAddress().state()) {
case "AL":
return new ALTaxCalculator().calculateTax(invoice, total);
case "NC":
return new NCTaxCalculator().calculateTax(invoice, total);
case "PA":
return new PATaxCalculator().calculateTax(invoice, total);
default:
// No sales tax
return 0.0;
}
}
}
interface TaxCalculator {
double calculate(Invoice invoice, double total);
}
class ALTaxCalculator implements TaxCalculator {
@Override
public double calculate(final Invoice invoice, final double total) {
return total * 0.04;
}
}
class NCTaxCalculator implements TaxCalculator {
@Override
public double caclulate(final Invoice invoice, final double total) {
return total * 0.0425;
}
}
class PATaxCalculator implements TaxCalculator {
@Override
public double calculate(final Invoice invoice, final double total) {
double taxable = 0.0;
for (Item item : invoice.items()) {
if (item.category() != Category.GROCERY &&
item.category() != Category.CLOTHING) {
taxable += item.price();
}
}
return (taxable * 0.06);
}
}
Now we can push that calculateTax call out of that method. We'll rename the method to signify the change in behavior. There is one wrinkle in this plan, though. For no sales tax, we don't do anything. We'll need a tax calculator to represent that.
public double calculateTotal(final Invoice invoice) {
double total = invoice.subtotal();
total += TaxCalculators.calculatorFor(invoice).calculateTax(invoice, total);
total += this.shippingService.calculateCost(invoice.shippingAddress());
return total;
}
class TaxCalculators {
public static TaxCalculator calculatorFor(final Invoice invoice) {
switch (invoice.shippingAddress().state()) {
case "AL":
return new ALTaxCalculator();
case "NC":
return new NCTaxCalculator();
case "PA":
return new PATaxCalculator();
default:
return new NoTaxCalculator();
}
}
}
interface TaxCalculator {
double calculate(Invoice invoice, double total);
}
class NoTaxCalculator implements TaxCalculator {
@Override
public double calculate(final Invoice invoice, final double total) {
return 0.0;
}
}
class ALTaxCalculator implements TaxCalculator {
@Override
public double calculate(final Invoice invoice, final double total) {
return total * 0.04;
}
}
class NCTaxCalculator implements TaxCalculator {
@Override
public double caclulate(final Invoice invoice, final double total) {
return total * 0.0425;
}
}
class PATaxCalculator implements TaxCalculator {
@Override
public double calculate(final Invoice invoice, final double total) {
double taxable = 0.0;
for (Item item : invoice.items()) {
if (item.category() != Category.GROCERY &&
item.category() != Category.CLOTHING) {
taxable += item.price();
}
}
return (taxable * 0.06);
}
}
The TaxCalculators class is looking a lot like a factory service. We can make it injectable. That would isolate the calculateTotal completely from the implementation of the tax calculators.
public double calculateTotal(final Invoice invoice) {
double total = invoice.subtotal();
total += this.taxCalculators.calculatorFor(invoice).calculateTax(invoice, total);
total += this.shippingService.calculateCost(invoice.shippingAddress());
return total;
}
class TaxCalculators {
public static TaxCalculator calculatorFor(final Invoice invoice) {
switch (invoice.shippingAddress().state()) {
case "AL":
return new ALTaxCalculator();
case "NC":
return new NCTaxCalculator();
case "PA":
return new PATaxCalculator();
default:
return new NoTaxCalculator();
}
}
}
interface TaxCalculator {
double calculate(Invoice invoice, double total);
}
class NoTaxCalculator implements TaxCalculator {
@Override
public double calculate(Invoice invoice, double total) {
return 0.0;
}
}
class ALTaxCalculator implements TaxCalculator {
@Override
public double calculate(final Invoice invoice, final double total) {
return total * 0.04;
}
}
class NCTaxCalculator implements TaxCalculator {
@Override
public double caclulate(final Invoice invoice, final double total) {
return total * 0.0425;
}
}
class PATaxCalculator implements TaxCalculator {
@Override
public double calculate(final Invoice invoice, final double total) {
double taxable = 0.0;
for (Item item : invoice.items()) {
if (item.category() != Category.GROCERY &&
item.category() != Category.CLOTHING) {
taxable += item.price();
}
}
return (taxable * 0.06);
}
}
We've now reached a really good place. We can add tax calculators by adding a new class and updating the factory service. We can be sure everything works through testing. We were able to get here by giving a name to the problem we had. By creating the idea of a tax calculator, we were able to leverage that into better code.
Comments
Post a Comment