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

Popular posts from this blog

The Timeline of Errors

Image
As software developers, we see errors every day. They manifest as exceptions or segfaults or error codes, telling us that our code has gotten into a state we didn't expect. Their appearance often portends bugs. Though we groan at an unexpected stack trace, we should see an error as a form of automated feedback . Feedback can be fast or slow. We can put the discovery of errors onto a timeline. Errors appear at many points over the lifetime of the code, starting at the moment it is compiled. The further to the right that an error appears, the longer it takes for the feedback to appear. Compile time Compile time is the earliest we can receive feedback about an error. The compiler automatically does a number of checks to make sure the code makes some semblance of sense. The most powerful tool for compile-time feedback is the type-checker. The type-checker makes sure that only values of the expected type are passed around to the places that expect them. This guards ...
Read more

Assuming the Bugs

Writing code is all about making assumptions. Sometimes those assumptions are explicit, but more often we make those assumptions implicitly. We make assumptions about what kinds of parameters we will receive. We make assumptions about what kinds of values methods and functions will return. We make assumptions about the global state. The correctness of our code relies on the correctness of these assumptions. Bad assumptions can wreck our applicatons. We assumed that method never returns null, but turns out it does. Now we have an unexpected NullPointerException crashing through our stack. We assume that we will always average at least one element. Turns out someone wants to average zero elements. Boom, division by zero, ArithmeticException . We make so many assumptions through our codebases, at least a few are going to be wrong. Really, there are many, many assumptions that are wrong. These are bugs. These bad assumptions are all of our bugs. Usually, our assumptions are i...
Read more

Assuming Debugging

Bugs are bad assumptions . Debugging is the process of testing our assumptions until we discover the invalid one(s). Once found, we can then correct those bad assumptions. The following code generates blog post author statistics for a popular blog site. The statistics show which days of the week authors are making posts. Most of the authors post every day, and extra on Fridays and Saturdays to capture the weekend audience. It has at least one bug. @GET @Path("/authors/{email}/weekly-stats") public UserWeeklyStats calculateWeeklyStats(@PathParam("email") final String email) { final User user = this.userService.byEmail(email); final Map<LocalDate, List<Post>> postsByDate = this.postService.thisWeekGroupedByDateFor(user.id()); if (postsByDate.isEmpty()) { return null; } final Map<DayOfWeek, List<Post>> postsByDayOfWeek = new HashMap<>(); postsByDate.forEach((date, post) -> post...
Read more