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:

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)

Tuesday, October 13, 2009

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.

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.

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:

There is an extra test needed, which in terms of maintence is not such a good thing.

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?