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:
- 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?
Thanks everyone who participated in the discussions!
