One of the attendees of the recent design improvement workshop that I ran sent me a question today. I've taken the liberty of changing the code a bit to demonstrate the example, but the intent is basically the same.
When we did the design improvement workshop we discussed how static methods can be a code smell. I'm wondering if you think this holds true for static factories since I don't feel right about creating a new instance of a factory just to ask it for an object ?
For example, I would normally have a factory like this:
public class ThingFactory { public static Thing create() { // ... } }So the calling code would look like this:
public class ThingProcessor { public void process() { Thing myThing = ThingFactory.create(); // ... } }I'm a little anxious about code that looks like this:
public class ThingProcessor { public void process() { Thing myThing = new ThingFactory().create(); // ... } }Since we create two object's and only one is kept.
The problem with static methods is that they make unit testing harder. You can't override them in a subclass, and you can't mock them out. So the implementation you've got is the only one that can be called. Here's how I would implement the factory from the example above:
public interface ThingFactory {
public Thing create();
}
public class DefaultThingFactory implements ThingFactory {
public Thing create() {
// ...
}
}
Now that I have an interface, I've got a mechanism that allows me to mock a method call. A useful trick to use in conjunction with that is to allow the actual implementation to be used by default in production code. You can do that with a combination of constructors like this:
public class ThingProcessor {
private final ThingFactory _factory;
// This constructor lets you inject a mock for testing.
public ThingProcessor(ThingFactory factory) {
_factory = factory;
}
// This constructor can be used by production code that just wants the default factory.
public ThingProcessor() {
this(new DefaultThingFactory());
}
public void process() {
Thing myThing = _factory.create();
// ...
}
}
The second design is much easier to test, yet the API for users of the ThingProcessor is exactly the same.
Comments
If a factory is a static method then there's no point of it existing at all. The whole point of the factory pattern(s) is the use of polymorphism to allow the creation of different types of object at runtime. There's no difference between "FactoryClass.createSomething()" and "new Something()" (assuming that the FactoryClass.createSomething method performs "new Something").
Posted by: Anonymous | April 14, 2007 12:19 AM
Well, unlike the anonymous comment, I do find a place for static factory methods. They allow for more flexibility than constructors, which do have some limitations (such as forcing you to know the exact class you want).
However, my preferred answer to testing classes that use them is: dependency injection. In particular, you can configure Spring to invoke the static factory method for you. (Alternatively, you can use a true factory class, configure it as a bean, then get Spring to call the factory methods on that bean)
Either way, the point is that you take out the direct static reference in the code and move it elsewhere (or eliminate it).
Posted by: Robert | April 14, 2007 07:43 AM
Consider if the factory allows injection of the actual factory provider? Therefore it's more of a facade than a real factory. Eg:
class ThingFactory implements Factory {
static void setProvider(Factory actualFactory) { ...
Now you can set a mock into it for testing and you can also have code reference the factory without having to create a new instance of it each time. eg:
ThingFactory.setProvider(aFactoryThatGoesToSpring);
:
Thing myThing = ThingFactory.create();
:
This is my preferred approach and yes there is the drawback that the factory provider is set globally for all, but I find it rarely presents a problem.
We are also hiding the dependency on Spring.
What do you think of this variation ?
Posted by: James Ladd | April 14, 2007 08:51 AM
This library enables mocking of static methods: http://jmockit.dev.java.net. I haven't actually used it but I could see that it would be useful for testing legacy code with extensive use of statics.
Robert:
I think a neater solution with Spring is to just treat your factory as a bean with an interface as Marty described. Then you can replace the factory easily using setter/constructor injection for unit testing and with bean definition overrides for integration testing.
Posted by: Tomas Varsavsky | April 14, 2007 10:39 AM
Robert - I agree with you about dependency injection, but DI != spring. There's no reason to say a factory can't be injected anyway. :)
James - I'm not sure of the difference to the original design. You've just pushed the problem one level deeper. Also, you shouldn't be depending on spring at all in the depths of your code. The only thing in your app that should know about spring is the entry point that fires it up. So using "aFactoryThatGoesToSpring" seems odd to me.
The spring discussions are all out of context for me. The refactoring I've proposed is to enable *unit* testing. Use of spring is an *integration* concern. All we want to do is maintain an API such that integration by whatever means is still convenient.
Posted by: Marty Andrews | April 16, 2007 01:02 PM
Hi Marty,
I fully agree with the approach you've outlined. However, I've run into opposition in the past because large systems having separate interfaces and implementations can have lots of classes.
Whilst both you and I know that the testing benefits far outweigh this cost, I've had experience in the past where both managers and junior developers see only the number of classes and reach the conclusion that it's overkill. They also complain that it makes it harder to browse through the code because when they jump into a method it takes them to an interface instead of the implementation they want.
In such circumstances, I've found that using class extensions to mock classes can be very useful. Both JMock and EasyMock support this. The only stipulation is that the classes being mocked need to have no-argument constructors - not an unreasonable requirement.
In fact, now I do all of my development using class extensions. That's not to say that I don't think carefully about the interfaces to my classes - it's just that, pragmatically speaking, it results in less code that is easier to navigate.
Cheers,
Ben
Posted by: Ben Teese | April 16, 2007 04:01 PM
Marty,
I see your points about Spring but a Console App with a main() will need to know about a Factory that uses Spring or use Spring directly, but please let me know if I have this wrong.
BTW - "DI != spring" is well said. I want that on a t-shirt :)
Rgs, James.
Posted by: James Ladd | April 16, 2007 05:45 PM
Ben - I feel for you if your managers and junior programmers are deciding how you should design your classes.
Systems should be a complex arrangement of simple classes, not a simple arrangement of complex classes.
Posted by: Marty Andrews | April 16, 2007 05:49 PM
James - any system that uses spring should have one (and only one) point in it that knows about spring. In a console app, that's the main method. In a web app, its a servlet. etc etc.
Your main method should initalise the system object graph from spring, and then let the system run. Consider the main() a wrapper around your system though, not the system itself. You should in theory be able to write a different main that initialises the system by hand. The system code should have no knowledge about the existence of a DI container.
Posted by: Marty Andrews | April 16, 2007 05:53 PM
Marty,
Thanks, you make it much more clear than my post. :)
Posted by: James Ladd | April 19, 2007 08:36 AM