Monday, July 20, 2009

Busy Constructors (are no problem to test)

For whatever reason, I was browsing Miško Hevery's old testability blog entries and found a nice top 10 list of developer practices that make code hard to test.

Too bad I totally disagree with Item #3: Doing work in constructor. There's nothing untestable about big long constructors. No, really. Consider this horrorshow constructor, in which a global DataSource is retrieved from a mingleton and a concrete class is instantiated:

import javax.sql.DataSource;

public class UserService {

private final DataSource dataSource;
private final UserSettings settings;

public UserService() {
dataSource = DatasourceFactory.getDefault();
settings = new UserSettingsImpl();
}
}
Don't get me wrong, this is a horrible design. For one, retrieving the data source from a global static method is an example of a mingleton, which is global state by any other name: near impossible to inject any sort of variable behavior or polymorphism into. And instantiating a concrete implementation class (new UserSettingsImpl()) tightly couples this service to that class, another undesirable trait. This service is incredibly tightly coupled to the dependencies it requires. You'll never be able to reuse this service in another context where the datasource and user settings are different. And writing a test would be reusing this service... so what possibly makes this easy to test?

Package local constructors mixed with an Extract Parameter refactoring is what makes this easy to test. Simply create a new constructor for the object and leave out a visibility modifier. This makes the method only visible to objects in the same package (like your unit test)! Then add all the fields created in the original constructor to the parameter list. Final fields are your friend here because then the compiler will check to make sure you did this correctly. If you want to get fancy, chain the original constructor call into the new one:
public class UserService {

private final DataSource dataSource;
private final UserSettings settings;

public UserService() {
this(DatasourceFactory.getDefault(), new UserSettingsImpl());
}

UserService(DataSource dataSource, UserSettings settings) {
this.dataSource = dataSource;
this.settings = settings;
}
}
This transformation preserves all the type signatures that were previously in the class, so all the clients that were compiled against the original code will continue to work just fine without recompiling. So even if the class you need to refactor is depended upon from everywhere, this is still a local refactoring. None of your refactor fearing coworkers even needs to know; it'll just be between us.

Did this buy us anything? Yes. All the dependencies of the class are now exposed for mocking and stubbing, without requiring changes to any calling code. Of course, your original constructor can't be tested, so if you have behavior or logic in the constructor that you need to test, then a little more creative refactoring is needed and it may not be supported with just automated IDE refactorings. Some might complain that we've modified the production code simply to enable testability... but to them I ask for alternatives. When weighing time commitments and code impacts, I find this "expose local constructor" refactoring to be of real benefit and frequently used.

4 comments:

ivylane said...

>>You'll never be able to reuse this service in another context where the datasource and user settings are different.

Sorry to disagree, but YAGNI...

If/when that happens, you change the code. (And since the paramterized ctor is not public, you'd have to do that anyway).

Or, to put it another way -- perfect is the enemy of good enough.

Hamlet D'Arcy said...

@ivylane By "reuse this service in another context" I mean reuse this service from a unit test where the datasource is _not_ a real connection to the database and the UserSettings are not the production defaults.

I reuse almost 100% of the objects I write... from test cases.

Walter Harley said...

I've been interested in reading your thoughts on testability, but it seems like some of the attributes that support good testability also make for poor readability. For example, loosely coupled objects make it easy to supply mocks for testing; but they also make it hard to see what's going on in the production code.

I suppose it could be argued that one shouldn't be trying to understand a system by looking at its individual modules of code, but at least for Java that's about all there is to look at. So I end up spending much of my time playing the "who in the world calls this constructor - oh, it's a factory method - well, who calls this factory method - oh, it's reflectively invoked by Spring - gee, I wonder which config file is being used" game.

I'd be interested in your thoughts on whether this dichotomy is real and how to reconcile it.

Hamlet D'Arcy said...

@Walter - I was going to reply to your comment but it got kinda long: http://bit.ly/b3L7A