Software craftsman, author of Software Craftsmanship: Professionalism Pragmatism Pride (http://leanpub.com/socra) and founder of the London Software Craftsmanship Community (LSCC). Sandro started coding at a very young age but just started his professional career years later, in 1996. He has worked for startups, software houses, product companies and a few international consultancy companies. Having worked as a consultant for the majority of his career, he had the opportunity to work in a good variety of projects, different languages, technologies and industries. Currently he is a director and a software craftsman at UBS Investment Bank, where he writes code, look after the quality of the applications, mentor developers and help teams around the world to get better at delivering quality software. Sandro is a DZone MVB and is not an employee of DZone and has posted 34 posts at DZone. You can read more from them at their website. View Full User Profile

Extract, Inject, Kill: Breaking Hierarchies (Part 2)

03.09.2012
| 3192 views |
  • submit to reddit

In part 1 of this post I explain the problems of using the template method in deep class hierarchies and how I went to solve it. Please read it before reading this post.

Here is a more concrete example in how to break deep hierarchies using the Extract, Inject, Kill approach. Imagine the following hierarchy.

public abstract class PrincingService {
    
    public double calculatePrice(ShoppingBasket shoppingBasket, User user, String voucher) {
        double discount = calculateDiscount(user);
        double total = 0;
        for (ShoppingBasket.Item item : shoppingBasket.items()) {
            total += calculateProductPrice(item.getProduct(), item.getQuantity());
        }
        total = applyAdditionalDiscounts(total, user, voucher);
        return total * ((100 - discount) / 100);
    }

    protected abstract double calculateDiscount(User user);

    protected abstract double calculateProductPrice(Product product, int quantity);

    protected abstract double applyAdditionalDiscounts(double total, User user, String voucher);

}

public abstract class UserDiscountPricingService extends PrincingService {

    @Override
    protected double calculateDiscount(User user) {
        int discount = 0;
        if (user.isPrime()) { 
            discount = 10;
        }
        return discount;
    }
}

public abstract class VoucherPrincingService extends UserDiscountPricingService {

    private VoucherService voucherService;

    @Override
    protected double applyAdditionalDiscounts(double total, User user, String voucher) {
        double voucherValue = voucherService.getVoucherValue(voucher);
        double totalAfterValue = total - voucherValue;
        return (totalAfterValue > 0) ? totalAfterValue : 0;
    }

    public void setVoucherService(VoucherService voucherService) {
        this.voucherService = voucherService;
    }
}

public class BoxingDayPricingService extends VoucherPrincingService {
    public static final double BOXING_DAY_DISCOUNT = 0.60;

    @Override
    protected double calculateProductPrice(Product product, int quantity) {
        return ((product.getPrice() * quantity) * BOXING_DAY_DISCOUNT);
    }
}

public class StandardPricingService extends VoucherPrincingService {

    @Override
    protected double calculateProductPrice(Product product, int quantity) {
        return product.getPrice() * quantity;
    }
}
Let's start with the StandardPricingService. First, let's write some tests:
public class StandardPricingServiceTest {

    private TestableStandardPricingService standardPricingService = new TestableStandardPricingService();
    
    @Test public void
    should_return_product_price_when_quantity_is_one() {
        Product book = aProduct().costing(10).build();

        double price = standardPricingService.calculateProductPrice(book, 1);

        assertThat(price, is(10D));
    }
    
    @Test public void
    should_return_product_price_multiplied_by_quantity() {
        Product book = aProduct().costing(10).build();

        double price = standardPricingService.calculateProductPrice(book, 3);

        assertThat(price, is(30D));
    }

    @Test public void
    should_return_zero_when_quantity_is_zero() {
        Product book = aProduct().costing(10).build();

        double price = standardPricingService.calculateProductPrice(book, 0);

        assertThat(price, is(0D));
    }

    private class TestableStandardPricingService extends StandardPricingService {
        @Override
        protected double calculateProductPrice(Product product, int quantity) {
            return super.calculateProductPrice(product, quantity);
        }
    }
}

Note that I used  a small trick here, extending the StandardPricingService class inside the test class so I could have access to the protected method. We should not use this trick in normal circumstances. Remember that if you feel the need to test protected or private methods, it is because your design is not quite right, that means, there is a domain concept missing in your design. In other words, there is a class crying to come out from the class you are trying to test.

Now, let's do the step one in our Extract, Inject, Kill strategy. Extract the content of the calculateProductPrice() method into another class called StandardPriceCalculation. This can be done automatically using IntelliJ or Eclipse. After a few minor adjusts, that's what we've got. 

public class StandardPriceCalculation {

    public double calculateProductPrice(Product product, int quantity) {
        return product.getPrice() * quantity;
    }
}

And the StandardPriceService now looks like this:

public class StandardPricingService extends VoucherPrincingService {

    private final StandardPriceCalculation standardPriceCalculation = new StandardPriceCalculation();

    @Override
    protected double calculateProductPrice(Product product, int quantity) {
        return standardPriceCalculation.calculateProductPrice(product, quantity);
    }
}

All your tests should still pass.


As we create a new class, let's add some tests to it. They should be the same tests we had for the StandardPricingService.
public class StandardPriceCalculationTest {

    private StandardPriceCalculation priceCalculation = new StandardPriceCalculation();
    
    @Test public void
    should_return_product_price_when_quantity_is_one() {
        Product book = aProduct().costing(10).build();

        double price = priceCalculation.calculateProductPrice(book, 1);

        assertThat(price, is(10D));
    }
    
    @Test public void
    should_return_product_price_multiplied_by_quantity() {
        Product book = aProduct().costing(10).build();

        double price = priceCalculation.calculateProductPrice(book, 3);

        assertThat(price, is(30D));
    }

    @Test public void
    should_return_zero_when_quantity_is_zero() {
        Product book = aProduct().costing(10).build();

        double price = priceCalculation.calculateProductPrice(book, 0);

        assertThat(price, is(0D));
    }

}

Great, one sibling done. Now let's do the same thing for the BoxingDayPricingService.

public class BoxingDayPricingServiceTest {
    
    private TestableBoxingDayPricingService boxingDayPricingService = new TestableBoxingDayPricingService();

    @Test public void
    should_apply_boxing_day_discount_on_product_price() {
        Product book = aProduct().costing(10).build();

        double price = boxingDayPricingService.calculateProductPrice(book, 1);

        assertThat(price, is(6D));
    }

    @Test public void
    should_apply_boxing_day_discount_on_product_price_and_multiply_by_quantity() {
        Product book = aProduct().costing(10).build();

        double price = boxingDayPricingService.calculateProductPrice(book, 3);

        assertThat(price, is(18D));
    }

    private class TestableBoxingDayPricingService extends BoxingDayPricingService {
        
        @Override
        protected double calculateProductPrice(Product product, int quantity) {
            return super.calculateProductPrice(product, quantity);
        }
        
    }
}

Now let's extract the behaviour into another class. Let's call it BoxingDayPricingCalculation.

public class BoxingDayPriceCalculation {
    public static final double BOXING_DAY_DISCOUNT = 0.60;

    public double calculateProductPrice(Product product, int quantity) {
        return ((product.getPrice() * quantity) * BOXING_DAY_DISCOUNT);
    }
}

The new BoxingDayPriceService is now

public class BoxingDayPricingService extends VoucherPrincingService {
    private final BoxingDayPriceCalculation boxingDayPriceCalculation = new BoxingDayPriceCalculation();

    @Override
    protected double calculateProductPrice(Product product, int quantity) {
        return boxingDayPriceCalculation.calculateProductPrice(product, quantity);
    }
}

We now need to add the tests for the new class.

public class BoxingDayPriceCalculationTest {
    
    private BoxingDayPriceCalculation priceCalculation = new BoxingDayPriceCalculation();

    @Test public void
    should_apply_boxing_day_discount_on_product_price() {
        Product book = aProduct().costing(10).build();

        double price = priceCalculation.calculateProductPrice(book, 1);

        assertThat(price, is(6D));
    }

    @Test public void
    should_apply_boxing_day_discount_on_product_price_and_multiply_by_quantity() {
        Product book = aProduct().costing(10).build(); 

        double price = priceCalculation.calculateProductPrice(book, 3);

        assertThat(price, is(18D));
    }

}

Now both StandardPricingService and BoxingDayPricingService have no implementation of their own. The only thing they do is to delegate the price calculation to StandardPriceCalculation and BoxingDayPriceCalculation respective. Both price calculation classes have the same public method, so now let's extract a PriceCalculation interface and make them both implement it.

public interface PriceCalculation {
    double calculateProductPrice(Product product, int quantity);
}

public class BoxingDayPriceCalculation implements PriceCalculation 

public class StandardPriceCalculation implements PriceCalculation 
Awesome. We are now ready for the Inject part of Extract, Inject, Kill approach. We just need to inject the desired behaviour into the parent (class that defines the template method). The calculateProductPrice() is defined in the PricingService, the class at the very top at the hierarchy. That's where we want to inject the PriceCalculation implementation. Here is the new version:
public abstract class PricingService {

    private PriceCalculation priceCalculation;

    public double calculatePrice(ShoppingBasket shoppingBasket, User user, String voucher) {
        double discount = calculateDiscount(user);
        double total = 0;
        for (ShoppingBasket.Item item : shoppingBasket.items()) {
            total += priceCalculation.calculateProductPrice(item.getProduct(), item.getQuantity());
        }
        total = applyAdditionalDiscounts(total, user, voucher);
        return total * ((100 - discount) / 100);
    }

    protected abstract double calculateDiscount(User user);

    protected abstract double applyAdditionalDiscounts(double total, User user, String voucher);

    public void setPriceCalculation(PriceCalculation priceCalculation) {
        this.priceCalculation = priceCalculation;
    }

}
Note that the template method calculateProductPrice() was removed from the PricingService, since its behaviour is now being injected instead of implemented by sub-classes.

As we are here, let's write some tests for this last change, checking if the PricingService is invoking the PriceCalculation correctly. 
Great. Now we are ready for the last bit of the Extract, Inject, Kill refactoring. Let's kill both StandardPricingService and BoxingDayPricingService child classes. 
The VoucherPricingService, now the deepest class in the hierarchy, can be promoted to concrete class. Let's have another look at the hierarchy: And that's it. Now it is just to repeat the same steps for VoucherPricingService and UserDiscountPricingService. Extract the implementation of their template methods into classes, inject them into PricingService, and kill the classes.

In doing so, every time you extract a class, try to give them proper names instead of calling them Service. Suggestions could be VoucherDiscountCalculation and PrimeUserDiscountCalculation.

There were a few un-safe steps in the re-factoring described above and I also struggled a little bit to describe exactly how I did it since I was playing quite a lot with the code. Suggestions and ideas are very welcome.

NOTE
If you are not used to use builders in your tests and is asking yourself where the hell aProduct() and aShoppingBasket() come from, check the code in here:

ProductBuilder.java
ShoppingBasketBuilder.java

For more information about the original problem that triggered all this, please read part 1 of this blog post.

 

From http://craftedsw.blogspot.com/2012/03/extract-inject-kill-breaking_06.html

Published at DZone with permission of Sandro Mancuso, author and DZone MVB.

(Note: Opinions expressed in this article and its replies are the opinions of their respective authors and not those of DZone, Inc.)

Comments

Jose Maria Arranz replied on Mon, 2012/03/12 - 9:52am

Sandro, there is nothing bad on class hierarchies and template methods when used properly.

Your example (I suppose is not yours because you don't like it)  is a typical example of bad design:

1) Discounts are cumulative, that is, different types of discounts must be modeled as a list in some way, only a very simple modeling of discount can be modeled using inheritance. This is not the case.

2) Inheritance must answer the question "It's a" as you say in the previous article (part I), as you can figure out, in this example StandardPrincingService and BoxyngDayPricingService hardly "can be" VoucherPricingService

I see your code like an example of trying to fix a bad use of inheritance, and yes it is ok, but if your objective is bashing implementation inheritance you are doing a very bad advice for unexperienced developers.

Regarding to testing, something is wrong if you bash implementation inheritance pursuing better testing, you must have very strong reasons to avoid implementation inheritance and better testing is not, testing is not "the thing". You can call protected/private methods using reflection, you can put the interesting code outside the method and test it, you can declare them as public and use interfaces in your code as an advanced approach of declaring public methods instead of the the poor man public/protected/private modifiers.

 I don't understand the current trend of bashing all the good stuff of OOP and trying to get us back to C or Visual Basic 6.

 

Sandro Mancuso replied on Tue, 2012/03/27 - 9:00am

Hi Jose Maria,

Thanks for your comments.

I definitely agree with you when you say that, when used correctly and respecting the "IS-A" relationship, inheritance and template methods are a valid approach. As you pointed out, in this post I'm addressing a poor design choice according to the problem at hand. Unfortunately I've seen it many times when working with legacy code. So, answering your question, no, I'm not bashing inheritance. I'm bashing the use of inheritance for the wrong reasons.

Regarding tests, I'm not saying that it is "the thing", as you mentioned. However, when I'm designing code, my priorities will always be to express the business domain, readability/maintainability and definitely testing. If an application design does not have testability in mind, I personally would not consider it a good and complete design.

From my perspective, protected / private methods are just implementation details of a public method. They are related to "HOW" things are done and not "WHAT" should be done. The public method defines the contract of the class, that means, how the other classes of the system will interact/collaborate with each other. Defining contract means defining WHAT the responsibility of that method is.

Well defined tests test WHAT and not HOW. Accessing and/or testing private and protected methods, via reflection or using one of the dodgy testing frameworks that allow you to do that, will make your tests brittle since they would be tied to the HOW things are done, straight jacketing your design. You should always be free to make changes to your implementation, that means, using private and protected methods, without breaking the contract. Private and protected methods are not important when it comes to class collaborations. 

In my view, protected and private methods should, as a consequence, be tested as part of the tests of the public methods. If, for some reason, you have a need to test a private or protected method in isolation, probably it is a sign that your public method (and your class) is doing too much, breaking the Single Responsibility Principle. That is also a sign that there is a domain concept crying to come out. In this case, that specific private or protected method should be extracted into another class as a public method and have this new class injected back into the original class. This new class would be named according to the domain concept that it represents, enriching your domain. 

When you say that I'm bashing the good OOP stuff, I would say that if you expose private and protected methods via reflection or in any other way, you would be breaking encapsulation, that is one of the "good stuff" of OOP that you were talking about. :)

Comment viewing options

Select your preferred way to display the comments and click "Save settings" to activate your changes.