Wednesday, October 28, 2009

Abstract Class Smell

PLEASE SEE THE UPDATES BELOW, I didn't realize that my issue was with the Template Method Pattern.
------------------------------------------------------------------------
Original Post:

I coded up a smell today, I had an abstract class who's public methods were calling the abstract implementations. Here's kind of what it looked like:

public abstract class Foo {
  abstract SomeObj foo();
  abstract SomeObj bar();
  public void somethingFooDoes() {
      SomeObj a = foo();
      SomeObj b = bar();
     //do some stuff with a + b
  }
}

How did I get here you might ask. Well, there were going to be two implementations of Foo, a prod implementation and a sandbox implementation. I started off with Foo as an interface and began coding up the Prod + Sandbox impls. It turned out that the prod and sandbox implementations had really similar structures. So, I made Foo an abstract class and moved the common stuff in there. A few more refactorings and Foo ended up looking like it does above. It then dawned on me, Foo was really using different strategies to get at the SomeObjs, I refactored to the strategy pattern:

public class Foo {
  private final SomeObjProducer someObjProducer;
  public Foo(SomeObjProducer someObjProducer) {
     this.someObjProducer = someObjProducer;
  }
  public void somethingFooDoes() {
      SomeObj a = someObjProducer.foo();
      SomeObj b = someObjProducer.bar();
     //do some stuff with a + b
  }
}

Now Foo has become concrete and is configurable with the different strategies for producing SomeObjs. To me this code is much more flexible and it now has collaborators. Where in the Abstract implementation it was kind of collaborating with itself (yuck!)

What's the moral of the story? If you have an abstract class and you find yourself calling abstract methods in your concrete methods (whoa that's a mouthful) consider refactoring to the strategy pattern.

UPDATE: a co-worker correctly pointed out that the first code snippet of Foo was the Template Method Pattern. Maybe it's too broad a statement, but it seems that anytime you have a template method pattern you could refactor it into the strategy pattern. Taking your inheritance based code and refactoring it into composition based code. Context is king, but these days I certainly favor composition over inheritance.

UPDATE AGAIN: I received some good complaints regarding my code sample.

Imagine using the more standard Game example from wikipedia (but simplified for brevity)

public abstract class Game {

 public void playOneGame() {
  while(!endOfGame()) {
   makePlay();
  }
 }

 public abstract boolean endOfGame();
 public abstract void makePlay();
}
This could be refactored to use composition instead of inheritance, where Game is now an interface.
public class GamePlayer {
 private Game game;
 public GamePlayer(Game game) {
  this.game = game;
 }

 public void playOneGame() {
  while(!game.endOfGame()) {
   game.makePlay();
  }
 }
}

Tuesday, October 13, 2009

Hollywood Dilemma

At sdtconf over the weekend I led an open spaces discussion about the Hollywood Principle and testing. Venkat Subramaniam took the time to post a really thoughtful article about it. I'll quote it below, but it is worth reading!

The discussion was around the cost and reason for inverting control, Venkat provided a toy problem which we twisted for our own purposes, we modeled a paper boy. The paper boy requests payment from a customer and the customer can either pay or stiff the paper boy.

I argued for the following:

public class PaperBoy {
  public void collectPayments(List customers) {
    for(Customer customer : customers) {
      customer.getPayment(this, 200);
    }
  }
  //...
And the test
Customer customer = mock(Customer.class);
PaperBoy paperBoy = new PaperBoy();
//set up a payment to be collected;
paperBoy.collectPayments(customer);
verify(customer).getPayment(paperBoy,$$$);
and the Customer
public class Customer {
  public void getPayment(PaperBoy paperBoy, int amount) {
    if (hasNoMoney || doesNotCareToPay)
      paperBoy.stiff(this);
    else {
      deductMoney(amount);
      paperBoy.acceptPayment(this, amount);
    }
  }
  //...
and the Test
Customer customer = new Customer(someMoney and a bad attitude);
PaperBoy paperBoy = mock(PaperBoy.class);
customer.getPayment(paperBoy,$$$);
verify(paperBoy).stiff(customer);
etc. for the other cases. Venkat points out some issues with this kind of implementation that I'd like to address:
Problem 1. The above code has some unnecessary coupling and bi-directional relationship. The Customer depends on PaperBoy. That is a coupling that I would seriously question.

Problem 2. The unit test is now made to do more. It has to create a mock of the PaperBoy. Even if you tell me it is not much code, a single line of code that is not absolutely needed is a lot of code IMHO. You don't have to maintain code that you do not write.

Problem 3. Are you really stiffing the PaperBoy? Who decides that? Can a paperboy decide that under very hard economic conditions, a loyal customer may be deserves a break? I have no problem the customer deciding to pay or not, but if that is stiffing or not is for the paperboy to decide IMO.

I'd like to address problem 3 first. The paper boy can still cut the guy a break, he just does it in his stiff method. Or maybe it shouldn't be a stiff method, maybe it should be an iWontPayMethod().

Problem 1. I really don't like the coupling! The coupling would be limited if the Customer was talking to a debt collector interface, and the PaperBoy was talking to a Payee(uhg) interface. But it's still a little smelly. I don't fundamentally see a problem with the bi-directional relationship, but maybe that's just because I haven't coded too much with bi-directional relationships (and maybe there's a good reason for that).

Problem 2. Let's take a look at Venkat's code + what some tests would look like.

public class PaperBoy {
  public void collectPayments(List customers) {
    for(Customer customer : customers) {
      int payment = customer.getPayment(200);
      if (payment > 0)
        acceptPayment(customer, payment);
      else
        stiff(customer);
    }
  }
  //...

public class Customer
{
  public int getPayment(int amount) {
    if (hasNoMoney || doesNotCareToPay)
      return 0;
    else {
      deductMoney(amount);
      return amount;
    }
  }
  //...
The Customer Tests:
Customer customer = new Customer(someMoney and a bad attitude);
assertEquals(0,customer.getPayment($$$);
So Venkat is correct in this example the mocks add 2 extra lines per test which isn't great. But that's not the whole story. Let's take a look at the PaperBoy tests.
Customer customer = new Customer(someMoney and a bad attitude);
PaperBoy paperBoy = new PaperBoy();
//set up a payment to be collected, let's assume this is one line
paperBoy.collectPayments(customer);
assertTrue(paperBoy.wasStiffedBy(customer);

Customer customer = new Customer(someMoney and a good attitude);
PaperBoy paperBoy = new PaperBoy();
//set up a payment to be collected, let's assume this is one line
paperBoy.collectPayments(customer);
assertTrue(paperBoy.collectedPaymentFrom(customer, $$$);
I have two issues with the PaperBoy tests:
  1. There is an extra test needed, which in terms of maintence is not such a good thing.
  2. The PaperBoy tests are highly coupled to the implementation of the Customer. This means anytime you change some behavior of the Customer you might have to change your PaperBoy test, not such a big problem for the 2 tests, but if there are 50? if there are 100? that can be bad news.

It's not clear to me that inverting the control is the "right" thing to do, but I think if you're careful (and you want 100% unit test coverage), inverting the control and using mocks can help you cut down on test complexity and test maintenance, so... mock-on?

Thanks everyone who participated in the discussions!

 
Web Statistics