As more programmers are getting excited about test driven development (TDD), one concern I am hearing more these days is "my tests and mocks make it hard to refactor code."
Remember we took a while to learn how to write better code. Similarly we will take a while to learn how to write better unit tests.
I want to talk about unit testing and mocking here with an example that was brought up yesterday at the (#sdtconf). A fine gentleman Zachary Shaw lead an open space discussion on the Hollywood Principle and mocking. A number of very bright programmers who're very passionate about TDD were in the session.
When talking about the principles, etc. Zack demanded a concrete example. We tossed around a few examples and I mentioned my friend David Bock's Paper Boy example. Taking that example and tweaking it to our discussion and desires (as we programmers often do), we ended up with something like this:
public class PaperBoy {
public void collectPayments(List customers) {
for(Customer customer : customers) {
customer.getPayment(this, 200);
}
}
//...
David, in his example, does not suggest that getPayment() accept an instance of PaperBoy, the example in our discussion deviated from his example rather quickly.
Now, how would the getPayment() method of the Customer look like?
public class Customer {
public void getPayment(PaperBoy paperBoy, int amount) {
if (hasNoMoney || doesNotCareToPay)
paperBoy.stiff(this);
else {
deductMoney(amount);
paperBoy.acceptPayment(this, amount);
}
}
//...
It was suggested in the discussion that, in getPayment() method, the customer may decide to pay or based on some condition may decide to stiff the paperboy.
So, how do we unit test this getPayment() method? Most of us readily agreed that we should mock-out (or stub out or spy out, it does not matter for our discussion here) the PaperBoy. That will help us run the unit test on the getPayment() method by isolating the PaperBoy.
Not so fast. Yes, if the getPayment() method has to be written like above, then mocking out PaperBoy may be one way to unit test it. However, there are at least three problem (that I can think of at the moment) with the above code.
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.
So, yes, you can certainly consider writing mocks for the PaperBoy, but I would not. Mocking is a tool I would use as the last resort when unit testing.
How would I approach the above getPayment() method? Let's take a look:
public class Customer
{
public int getPayment(int amount) {
if (hasNoMoney || doesNotCareToPay)
return 0;
else {
deductMoney(amount);
return amount;
}
}
//...
Here the payment method does not depend on the PaperBoy. (If necessary you can pass information on what the payment is for-paper, magazine, lawn moving, ...). The customer focuses on his/her own logic, whether to pay or not.
Now, how would the PaperBoy's collectPayment look like?
public class PaperBoy {
public void collectPayments(List customers) {
for(Customer customer : customers) {
int payment = customer.getPayment(200);
if (payment == 0)
if (payment > 0)
acceptPayment(customer, payment);
else
stiff(customer);
}
}
//...
Our paperboy thinks that if payment has not been received, he's been stiffed. He could change his view at anytime without affecting the customer class. The paperboy is a bit more complex now, but that decision as to whether he's been stiffed or not belongs to him, not the customer.
Now, how would we unit test the getPayment() method of the Customer. That's not a big deal. No mocking is needed. Simply run your test on your customer instance.
If you think returning 0 for payment amount is not clear enough, you can return a Payment object with more details contained in it like Cash, CreditCard, BrokeCantPay, DontFeelLikePaying, etc. as the type of payment. It can then include additional details like credit card number, etc. or you can even extend the payment information object. That can be used for any payment, not just payment for paper. The decoupling also makes the code more extensible.
So, when would I use a mock or a stub? I would first try to remove or reduce dependency as much as I can. After a serious effort to do that, then for those dependencies that are still left I would do one of the following:
A. Separate the business logic in my code from the object it collaborates with. That way, I can run unit test on the logic. Depending on the situation, I am quite content with integration testing the code that collaborate and unit test the code that does the important logic. I do not think 100% of the code needs to be unit tested. I do think that 100% of the code should be exercised. So, if I can get coverage of the code that does business logic through unit test and coverage of the
code that collaborates, with the object it depends on, through integration tests, I'm quite happy assuming it gives me a reasonable (not necessarily instantaneous) feedback loop.
B. If that separation is hard or if I think I need to really get a quick feedback on the code while testing the collaboration not just logic, then and only then will I consider writing a mock.
In short, I use mock as the last option when I do TDD. I will first try to knock it out. Only if I really can't I will try to mock it out.
First try to knockout before you consider to mock-out.
Comment by Puneet Sarda (http://www.twitter.com/puneetsarda)
Once again SRP to rescue. In the initial case customer was responsible for both deciding whether to pay or not as well changing the behavior of paperboy based on that action. Now that the customer is responsible for just the first part and the paperboy gets to decide if hes wants to process the order or not, the rest of the stuff falls in place.
I love the point: a single line of code that is not absolutely needed is a lot of code IMHO.
Comment by James Estes
Looks like a logic error in the last code snippet. The paperboy thinks hes stiffed if he gets paid ;)
I agree with your stance on mocks. I feel like we overuse them and I too see them making us more complacent about design and rigorous reduction of coupling. I'm sure there's a violation in there screaming to be pointed out, but for now I'll just call it laziness (myself included) and an over confidence in tests...just HAVING a test. That overconfidence paired with how mocks make the coupling less painful in tests, lead to reduced questioning of how to improve the design.
Comment by Jesse Wolgamott (http://jessewolgamott.com)
Agree with James on the slight logic flaw.
Probably should have been:
if (payment == 0) ---> if (payment >= 0)
I agree with the premise... just having the ability to mock doesnt make it right. We should still isolate rules to make things easier to test.
Comment by Venkat Subramaniam (http://www.agiledeveloper.com)
Thanks guys! Note to self: Don't post message in a hurry when the airplane door is about to close!
Comment by Shih-gian Lee
This is very enlightening. I have identified the Customer's getPayment() does too much after reading through 1/3 of your blog. The getPayment() has extra information that it should not have i.e. deciding the fate of a paperboy. I am glad we think alike ;)
Comment by Curtis Cooley (http://curtiscooley.com)
Just a little word of warning. Don't get too caught up in "modeling the real world" I was helping teach an OOD class once. We were doing the typical payroll example with different kinds of employees getting paid in different kinds of ways. One group wanted to put a pay() method on an Employee interface and have different types of employee implement it, but one member just could not get past the fact that employees don't pay themselves. Sure, we pay attention to the real world, but don't let it have too much say in the model.
Comment by David Linsin (http://dlinsin.blogspot.com)
Greate example of how to reduce dependencies by thinking about testing! I believe that with testability in mind you can come up with better code!
