Monday, 26 March 2012

Extract, Inject, Kill: Breaking hierarchies (part 3)

In part one I explain the main idea behind this approach and in part two I start this example. Please read parts one and two before reading this post

Although the main ideas of Extract, Inject, Kill is already expressed, it's good to finish the exercise just for completion's sake. Here is where we stopped:

Let's have a look at the VoucherPricingService, that now is the only concrete class at the bottom of our hierarchy. 

public class VoucherPricingService 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;
}
}

Note that it uses the VoucherService class to calculate the voucher value.

public class VoucherService {
public double getVoucherValue(String voucher) {
// Imagine that this calculate the voucher price.
// Keeping it simple so we can understand the approach.
return 0;
}
}
Before anything, let's write some tests to VoucherPricingService.java

@RunWith(MockitoJUnitRunner.class)
public class VoucherPricingServiceTest {
private static final User UNUSED_USER = null;
private static final String NO_VOUCHER = null;
private static final String TWENTY_POUNDS_VOUCHER = "20";
@Mock private VoucherService voucherService;
private TestableVoucherPricingService voucherPricingService;
@Before
public void initialise() {
voucherPricingService = new TestableVoucherPricingService();
voucherPricingService.setVoucherService(voucherService);
when(voucherService.getVoucherValue(TWENTY_POUNDS_VOUCHER)).thenReturn(20D);
}
@Test public void
should_not_apply_discount_if_no_voucher_is_received() {
double returnedAmount = voucherPricingService.applyAdditionalDiscounts(1000, UNUSED_USER, NO_VOUCHER);
assertThat(returnedAmount, is(1000D));
}
@Test public void
should_subtract_voucher_value_from_total() {
double returnedAmount = voucherPricingService.applyAdditionalDiscounts(30D, UNUSED_USER, TWENTY_POUNDS_VOUCHER);
assertThat(returnedAmount, is(equalTo(10D)));
}
@Test public void
shoudl_return_zero_if_voucher_value_is_higher_than_total() {
double returnedAmount = voucherPricingService.applyAdditionalDiscounts(10D, UNUSED_USER, TWENTY_POUNDS_VOUCHER);
assertThat(returnedAmount, is(equalTo(0D)));
}
private class TestableVoucherPricingService extends VoucherPricingService {
@Override
protected double applyAdditionalDiscounts(double total, User user, String voucher) {
return super.applyAdditionalDiscounts(total, user, voucher);
}
}
}



Once thing to notice is that the User parameter is not used for anything. So let's remove it.

Now it is time to user the Extract, Inject, Kill on the VoucherPricingService. Let's Extract the content of the VoucherPricingService.applyAdditionalDiscounts(double, String) method and add it to a class called VoucherDiscountCalculation. Let's call the method calculateVoucherDiscount(). Of course, let's do that writing our tests first. They need to test exactly the same things that are tested on VoucherPricingService.applyAdditionalDiscounts(double, String). We also take the opportunity to pass the VoucherService object into the constructor of VoucherDiscountCalculation.

@RunWith(MockitoJUnitRunner.class)
public class VoucherDiscountCalculationTest {
private static final String NO_VOUCHER = null;
private static final String TWENTY_POUNDS_VOUCHER = "20";
@Mock
private VoucherService voucherService;
private VoucherDiscountCalculation voucherDiscountCalculation;
@Before
public void initialise() {
voucherDiscountCalculation = new VoucherDiscountCalculation(voucherService);
when(voucherService.getVoucherValue(TWENTY_POUNDS_VOUCHER)).thenReturn(20D);
}
@Test public void
should_not_apply_discount_if_no_voucher_is_received() {
double returnedAmount = voucherDiscountCalculation.calculateVoucherDiscount(1000, NO_VOUCHER);
assertThat(returnedAmount, is(1000D));
}
@Test public void
should_subtract_voucher_value_from_total() {
double returnedAmount = voucherDiscountCalculation.calculateVoucherDiscount(30D, TWENTY_POUNDS_VOUCHER);
assertThat(returnedAmount, is(equalTo(10D)));
}
@Test public void
should_return_zero_if_voucher_value_is_higher_than_total() {
double returnedAmount = voucherDiscountCalculation.calculateVoucherDiscount(10D, TWENTY_POUNDS_VOUCHER);
assertThat(returnedAmount, is(equalTo(0D)));
}
}
public class VoucherDiscountCalculation {
private VoucherService voucherService;
public VoucherDiscountCalculation(VoucherService voucherService) {
this.voucherService = voucherService;
}
public double calculateVoucherDiscount(double total, String voucher) {
double voucherValue = voucherService.getVoucherValue(voucher);
double totalAfterValue = total - voucherValue;
return (totalAfterValue > 0) ? totalAfterValue : 0;
}
}
If you noticed, when doing the extraction, we took the opportunity to give proper names to our new classes and methods and also to pass their essential dependencies to the constructor instead of using method injection.
 
Let's now change the code in the VoucherPricingService to use the new VoucherDiscountCalculation and see if all the tests still pass.

public class VoucherPricingService extends UserDiscountPricingService {
private VoucherService voucherService;
@Override
protected double applyAdditionalDiscounts(double total, String voucher) {
VoucherDiscountCalculation voucherDiscountCalculation = new VoucherDiscountCalculation(voucherService);
return voucherDiscountCalculation.calculateVoucherDiscount(total, voucher);
}
public void setVoucherService(VoucherService voucherService) {
this.voucherService = voucherService;
}
}

Cool. All the tests still pass, meaning that we have the same behaviour, but now in the VoucherDiscountCalculation class, and we are ready to move to the Inject stage.

Let's now inject VoucherDiscountCalculation into PricingService, that is the top class in the hierarchy. As always, let's add a test that will test this new collaboration.

@RunWith(MockitoJUnitRunner.class)
public class PricingServiceTest {
private static final String NO_VOUCHER = "";
private static final String FIVE_POUNDS_VOUCHER = "5";
private TestablePricingService pricingService = new TestablePricingService();
private ShoppingBasket shoppingBasket;
@Mock private PriceCalculation priceCalculation;
@Mock private VoucherDiscountCalculation voucherDiscountCalculation;
@Before
public void initialise() {
this.pricingService.setPriceCalculation(priceCalculation);
this.pricingService.setVoucherDiscountCalculation(voucherDiscountCalculation);
}
@Test public void
should_calculate_price_of_all_products() {
Product book = aProduct().named("book").costing(10).build();
Product kindle = aProduct().named("kindle").costing(80).build();
shoppingBasket = aShoppingBasket()
.with(2, book)
.with(3, kindle)
.build();
double price = pricingService.calculatePrice(shoppingBasket, new User(), NO_VOUCHER);
verify(priceCalculation, times(1)).calculateProductPrice(book, 2);
verify(priceCalculation, times(1)).calculateProductPrice(kindle, 3);
}
@Test public void
should_calculate_voucher_discount() {
Product book = aProduct().named("book").costing(10).build();
when(priceCalculation.calculateProductPrice(book, 2)).thenReturn(20D);
shoppingBasket = aShoppingBasket()
.with(2, book)
.build();
double price = pricingService.calculatePrice(shoppingBasket, new User(), FIVE_POUNDS_VOUCHER);
verify(voucherDiscountCalculation, times(1)).calculateVoucherDiscount(20, FIVE_POUNDS_VOUCHER);
}
private class TestablePricingService extends PricingService {
@Override
protected double calculateDiscount(User user) {
return 0;
}
@Override
protected double applyAdditionalDiscounts(double total, String voucher) {
return 0;
}
}
}
And here is the changed PriningService.

public abstract class PricingService {
private PriceCalculation priceCalculation;
private VoucherDiscountCalculation voucherDiscountCalculation;
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 = voucherDiscountCalculation.calculateVoucherDiscount(total, voucher);
return total * ((100 - discount) / 100);
}
protected abstract double calculateDiscount(User user);
protected abstract double applyAdditionalDiscounts(double total, String voucher);
public void setPriceCalculation(PriceCalculation priceCalculation) {
this.priceCalculation = priceCalculation;
}
public void setVoucherDiscountCalculation(VoucherDiscountCalculation voucherDiscountCalculation) {
this.voucherDiscountCalculation = voucherDiscountCalculation;
}
}
Now it is time to kill the VoucherPricingService class and kill the PricingService.applyAdditionalDiscounts(double total, String voucher) template method, since it is not being used anymore. We can also kill the VoucherPricingServiceTest class and fix the PricingServiceTest removing the applyAdditionalDiscounts() method from the testable class.

So now, of course, we don't have a concrete class in our hierarchy anymore, since the VoucherPricingService was the only one. We can now safely promote UserDiscountPricingService to concrete.

That is now how our object graph looks like:


Our hierarchy is another level short. The only thing we need to do now is to apply  Extract, Inject, Kill once again, extracting the logic inside UserDiscountPricingService into another class (e.g. UserDiscountCalculation), inject UserDiscountCalculation into PricingService, finally kill UserDiscountPricingService and the calculateDiscount(User user) template method.  UserDiscountPricingService

Since the approach was described before, there is no need to go step by step anymore. Let's have a look at the final result.

Here is the diagram representing where we started:

  After the last Extract, Inject, Kill refactoring, this is what we've got:


The cool thing about the final model pictured above is that now we don't have any abstract classes anymore. All classes and methods are concrete and every single class is independently testable. 


That's how the final PricingService class looks like:
public class PricingService {
private PriceCalculation priceCalculation;
private VoucherDiscountCalculation voucherDiscountCalculation;
private PrimeUserDiscountCalculation primeUserDiscountCalculation;
public PricingService(PriceCalculation priceCalculation, VoucherDiscountCalculation voucherDiscountCalculation,
PrimeUserDiscountCalculation primeUserDiscountCalculation) {
this.priceCalculation = priceCalculation;
this.voucherDiscountCalculation = voucherDiscountCalculation;
this.primeUserDiscountCalculation = primeUserDiscountCalculation;
}
public double calculatePrice(ShoppingBasket shoppingBasket, User user, String voucher) {
double total = getTotalValueFor(shoppingBasket);
total = applyVoucherDiscount(voucher, total);
return totalAfterUserDiscount(total, userDiscount(user));
}
private double userDiscount(User user) {
return primeUserDiscountCalculation.calculateDiscount(user);
}
private double applyVoucherDiscount(String voucher, double total) {
return voucherDiscountCalculation.calculateVoucherDiscount(total, voucher);
}
private double totalAfterUserDiscount(double total, double discount) {
return total * ((100 - discount) / 100);
}
private double getTotalValueFor(ShoppingBasket shoppingBasket) {
double total = 0;
for (ShoppingBasket.Item item : shoppingBasket.items()) {
total += priceCalculation.calculateProductPrice(item.getProduct(), item.getQuantity());
}
return total;
}
}


For a full implementation of the final code, please look at https://github.com/sandromancuso/breaking-hierarchies

Note: For this three part blog post I used three different approaches to drawing UML diagrams. By hand, using ArgoUML and Astah community edition. I'm very happy with the latter. 

Tuesday, 6 March 2012

Extract, Inject, Kill: Breaking hierarchies (part 2)

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.

For the final solution, please check the last part of this blog post.

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.
In part 3 I finish the exercise, breaking the entire hierarchy. Please have a look at it for the final solution

Extract, Inject, Kill: Breaking hierarchies (part 1)


Years ago, before I caught the TDD bug, I used to love the template method pattern. I really thought that it was a great way to have an algorithm with polymorphic parts. The use of inheritance was something that I had no issues with. But yes, that was many years ago.

Over the years, I've been hurt by this "design style". That's the sort of design created by developers that do not TDD.

The situation

Very recently I was working on one part of our legacy code and found a six level deep hierarchy of classes. There were quite a few template methods defined in more than one of the classes. All classes were abstract with the exception of the bottom classes, that just implemented one or two of the template methods. There were just a single public method in the entire hierarchy, right at the very top.

We had to make a change in one of the classes at the bottom. One of the (protected) template method implementations had to be changed.

The problem

How do you test it? Goes without saying that there were zero tests for the hierarchy.

We know that we should never test private or protected methods. A class should "always" be tested from its public interface. We should always write tests that express and test "what" the method does and not "how". That's all well and good. However, in this case, the change needs to be done in a protected method (template method implementation) that is part of the implementation of a public method defined in a class six level up in the hierarchy. To test this method, invoking the public method of its grand grand grand grand parent we will need to understand the entire hierarchy, mock all dependencies, create the appropriate data, configure the mocks to have a well defined behaviour so that we can get this piece of code invoked and then tested.

Worse than that, imagine that this class at the bottom has siblings overriding the same template method. When the siblings need to be changed, the effort to write tests for them will be the same as it was for our original class. We will have loads of duplications and will also need to understand all the code inside all the classes in the hierarchy. The ice in the cake: There are hundreds of lines to be understood in all parent classes.

Breaking the rules

Testing via the public method defined at the very top of the hierarchy has proven not to be worth it. The main reason is that, besides painful, we already knew that the whole design was wrong. When we look at the classes in the hierarchy, they didn't even follow the IS-A rule of inheritance. They inherit from each other so some code could be re-used.

After some time I thought: Screw the rules and this design. I'm gonna just directly test the protected method and then start breaking the hierarchy.

The approach: Extract, Inject, Kill

The overall idea is:

1. Extract all the behaviour from the template method into a class.
2. Inject the new class into the parent class (where the template is defined), replacing the template method invocation with the invocation of the method in the new class.
3. Kill the child class (the one that had the template method implementation).

Repeat these steps until you get rid of the entire hierarchy.

This was done writing the tests first, making the protected template method implementation public.

NOTES
1. This may not be so simple if we have methods calling up the stack in the hierarchy.
2. If the class has siblings, we have to extract all the behaviour from the siblings before we can inject into the parent and kill the siblings.

This probably is too complicate to visualise, so in part 2 of this post I'll be giving a more concrete example