In response to a recent comment from Kalinowski regarding my idea for a “Legaciness” metric, I have decided try and boil down Michael Feathers’ legacy code patterns into a small list of bad programming practices that can make Java (and other) classes hard to unit test.
Patterns
First, a quick rundown of the patterns:
Adapt Parameter
If you have a method that takes a parameter that is hard to simulate, like HttpServletRequest in Java web apps, one solution (other than using mock libraries) would be to extract only the necessary part into a wrapper class. For example, if you only need to know a single querystring parameter, you can create a decorator class that only returns the single parameter. This would then be much easier to mock or fake than to have to override 30 or so methods that are doing things that are irrelevant to your test.
public void before(HttpServletRequest request) {
//get params from request
}
public void after(MyParamsSource params) {
//get params from wrapper class
}
Another reason you may want to implement this pattern is if the parameter simply CAN’T be mocked or faked. This would be true in the case of a concrete final class, for instance.
Break Out Method Object
Move a method that you want to test into a new, separate class. Local method variables can become instance variables of the new class, making it easier to substitute and break dependencies. This approach can be used when it’s nearly impossible to instantiate the class for testing, and refactoring all the dependencies would be too risky. While “Expose Static Method” (see below) is preferred for small methods which don’t have dependencies, “Break Out Method Object” can be used for the tougher cases.
public class UntestableClass {
private BreakOutObject boo;
public void untestableMethod() {
boo.testableMethod();
}
}
public class BreakOutObject {
public void testableMethod() {
//Do stuff we can test
}
}
The key here, in terms of bad practices, is that the class you want to test for some reason is “impossible to instantiate” in a unit test.
Expose Static Method
Turn a method that doesn’t depend on instance variables or methods into a static method. This will allow you to test the method without having to instantiate the class. You can use this when you are unable to instantiate a class for testing, but the method you want to test doesn’t depend on any of the instance variables in the class.
public class UninstantiableClass {
public void untestableMethod() {
UninstantiableClass.testableStaticMethod();
}
public static void testableStaticMethod() {
//Do stuff we can test
}
}
Again, this pattern is a workaround for classes that are difficult or impossible to instantiate in a unit test. See the discussion in “Break Out Method Object” for practices that may necessitate this.
Extract and Override Call
Extract calls to a static method to a protected internal method, then override the method in a subclass. The unit test may then test the subclass instead of the original class. This pattern may need to be used when a static method call has dependencies that cannot be resolved in a unit test.
public class Foo {
public void methodToTest() {
//Do stuff
extractedCall();
//Do more stuff
}
/** Override me in tests */
protected void extractedCall() {
UtilClass.evilStaticMethod();
}
}
As I have mentioned before, not all static method calls are necessarily problematic for unit testing. For example, a StringUtils-type method for converting a String to all caps, or a method for accessing an attribute via reflection wouldn’t prevent one from setting up an appropriate test case and executing the method. The key here is that the static method has dependencies that are hard to satisfy in a unit test, or does things that violate certain principles of unit testing.
Extract and Override Factory Method
Extract the instantiation of objects to a protected internal method, then override the method in a subclass. Test the subclass instead of the original class. This is related to “Extract and Override Call”, but is specifically for code related to the instantiation of objects.
public class Foo {
public void methodToTest() {
//Do stuff
bar = extractedCall();
//Do more stuff
}
/** Override me in tests to return subclass */
protected TypeToMock extractedCall() {
return new TypeToMock();
}
}
Calls to “new” and calls to static factory methods (which don’t let you substitute your own implementation), including the getInstance() method common in Singleton classes, all prevent the unit tester from substituting the real class a mock or fake implementation. So the bad practice here would be to avoid instantiating objects directly in a method, or via a static factory method call that doesn’t permit substitutes. The alternative would seem to be to use a Dependency Injection approach, which generally leads to more testable classes, but there are other solutions as well.
However, to say that one should NEVER use “new” is a lot like saying you should ALWAYS use interfaces for your classes. We need to be practical about this. There’s nothing wrong with instantiating an object directly in your code (from a testing perspective) if the object doesn’t have any of those unwanted dependencies we’ve been talking about. It would be pure dogma to penalize someone for making a call to new Integer(myIntVal), for instance.
Extract Implementer
Turn an implementation class into an interface and move the implementation into a new, separate class. You can use this on any class that is passed as a parameter or used as a member attribute in your testing class in order to permit the substitution of different implementations. Note that if you are using a mock object proxy via a testing framework such as EasyMock, you don’t actually have to do this. The concrete class extension to EasyMock lets you create proxies of any concrete class. Note that you should use this pattern only when you think it would be a good idea to create an interface for your class regardless of testing…
public class WantToMock {} --> public interface WantToMock{}, public class WantToMockImpl implements WantToMock {}
The lesson here is that even objects with simple interfaces passed as a method parameter can be a problem for testing if they are hard to set up, or if they do unwanted things to your test environment. If the class is in your code base, then you have the option of extracting the nasty implementation details out into a separate implementation class. For testing, you can then slip in a mock or a fake object which only mimics the parts that are important to your test.
There is a second lesson here as well, which could complicate the calculation of a Legaciness metric (should it attempt to take this into account): the technology available may influence exactly how hard it is to write your unit tests. Early versions of EasyMock, for example, did not come with an extension that made it possible to mock concrete classes; only interfaces could be simulated. Nowadays, this is no longer a problem, although in my experience, the use of a mock library such as this is already more difficult than being able to write my own simple test classes (note that there’s a big difference between mock objects and fake objects). More recently, JUnit and other test frameworks have begun to allow the use of aspects for testing. I haven’t personally used these capabilities, and I’m not sure what all can be done with these new tools, but the point is that testing technologies are a moving target.
Extract Interface
Create an interface for your implementation class and change all parameters and attributes that reference the old class to use the interface. You can use this on any class that is passed as a parameter or used as a member attribute in your testing class in order to permit the substitution of different implementations. The difference between this method and “Extract Implementer” is that the interface itself will have a different name from the old class. Use this when you prefer to create a new generic name for the interface.
public class WantToMock {} --> public interface Mockable{}, public class WantToMock implements Mockable {}
Note that this is purely an application design decision, unrelated to unit testing. Otherwise, the implications of this pattern are the same as for its sister pattern.
Introduce Instance Delegator
If the method you want to test makes static method calls, you can define an instance version of the method on the external class which delegates the call to its static method. The idea here is that your class can then call the instance method on the class instead, which can then be mocked or overridden. This assumes, of course, that you also find a way to access an instance of the object without having to call “new”. This seems like a pretty radical change to the main application design to resolve what is a testing problem. Michael Feathers apparently prefers instance objects over static utility classes as a general practice.
public class InstantiableUtil {
public static void staticFoo() {
//Do something bad
}
/** Call me instead. Override in test class. */
public void overridableFoo() {
InstantiableUtil.staticFoo();
}
}
At any rate, there’s nothing new we can learn from this pattern. The idea is that at least some static method calls are bad.
Introduce Static Setter
Add a static setter method to a singleton class that allows you to replace the unique instance. This can be used when your classes depend on a singleton class and you wish to alter its behavior for testing purposes. This method can also be used by “Global Factories”. If you have a factory class that returns objects via a static method, you can let your tests configure a mock or fake implementation via a Static Setter.
public class SingletonClass {
private static SingletonClass instance = new SingletonClass();
private SingletonClass() {}
public static SingletonClass getInstance() {
return instance;
}
/** This method should only be called by unit tests */
public static void setInstanceForTesting(SingletonClass testInstance) {
instance = testInstance;
}
}
This is a pretty good alternative to the “Extract and Override Factory Method” pattern. However, it can be quite dangerous to use, as it gives any class access to the inner workings of your factory classes. At any rate, it’s the same old story here: not being able to substitute your dependencies with a mock or fake is a Bad Thing for unit testing.
Parameterize Constructor
Define a constructor for your class that allows tests to supply their own implementations of instance attributes. This is a good candidate to use when your class instantiates its attributes in the constructor.
public class ExternalDependencies {
private BadDependency badDep1;
private BadDependency badDep2;
public ExternalDependencies() {
badDep1 = new BadDependency();
badDep2 = DoSomethingBadFactory.create();
}
/** Use me for tests */
public ExternalDependencies(BadDependency badDep1, BadDependency badDep2) {
this.badDep1 = badDep1;
this.badDep2 = badDep2;
}
}
As with many of these other patterns, this technique breaks encapsulation in the name of unit testing, so it can be dangerous. This pattern can be used to attack two different bad practices: unwanted dependencies caused by the use of “new”, and classes that can’t be instantiated for testing because they do bad things in the constructor.
Parameterize Method
Create a version (or change the existing version) of the method you are testing which accepts internal dependencies as parameters. If you create a second version, you can rewrite the original version to instantiate is variables as before, and then delegate the rest of the work to the new parameterized version.
public class NowCanMock {
private Foo foo;
public void doSomething() {
doSomething(foo);
}
/** Call this version in tests, and send it a mock Foo */
public void doSomething(Foo foo) {
//Do something
}
}
Pull Up Feature
Move dependency-free methods into an abstract superclass, then create a subclass of this abstract class that can be used in testing these methods. This is especially useful when you have a group of methods that can be tested and you’d prefer not to make them static or put them in a separate (unrelated) class. It is especially appropriate when the methods are seemingly unrelated, since related methods might better be refactored into a separate class vis-à-vis “Break Out Method Object”.
public class UninstantiableClass extends AbstractParentClass {
//Other methods
}
/** Subclass me with concrete class for testing */
public class AbstractParentClass {
public void testableMethod() {
//Do stuff we want to test
}
}
Replace Global Reference with Getter
Extract references to global objects (singletons and objects provided by static calls) into a protected method. Then use “Subclass and Override Method” to replace the object with another in tests. You can use this with any call to retrieve a global object. If the object is used in many places, you might want to consider a more general solution such as “Introduce Static Setter” or refactoring the object so it is no longer global.
public class Foo {
public void methodToTest() {
//Do stuff
ref = getGlobalReference();
//Do more stuff
}
/** Override me in tests to return subclass */
protected TypeToMock getGlobalReference() {
return GlobalClass.getReference();
}
}
Subclass and Override Method
Extract bad dependencies or code into separate isolated methods (if they aren’t already). Create a subclass for testing and override these methods to remove the problem. This technique can be used on any internal method of the class which is not an essential part of the logic being tested. In cases where only a part of the method should be overridden, that part can be extracted into its own protected method.
public class Foo {
public void methodToTest() {
//Do stuff
methodToOverride();
//Do more stuff
}
/** Override me in tests to return subclass */
protected void methodToOverride() {
//This is too long to wait in a unit test!
Thread.sleep(60000);
}
}
From the example, you can see that there are times when the code does something untestable even without there being an unwanted dependency.
Supersede Instance Variable
Add a method (i.e. a “setter”) to your class that allows you to replace the standard value of an attribute with one of your own choosing. This is a very easy way to insert fake and mock objects into your class for testing. However, it is also very intrusive, so it should be used with care. If you are using Spring or another IoC framework, it is very common to offer setters for all dependencies (with the alternative approach being “Parameterize Constructor”). When this approach is used, it doesn’t prevent the original dependency from being instantiated. The test waits until after it has been initialized to replace the original value. If the dependency prevents you from instantiating your class from testing, an alternative approach must be used.
public class Foo {
private Attribute bar;
private Attribute baz;
public Foo() {
bar = new Attribute(1);
baz = new Attribute(2);
}
public void setBarForUnitTesting(Attribute bar) {
this.bar = bar;
}
public void setBazForUnitTesting(Attribute baz) {
this.baz = baz;
}
}
Once again, this solution breaks encapsulation and “best practices” for OO. Another approach to this problem would be to use reflection to set the value of a private attribute (yes, in Java, it’s possible to do this). It may require a little more work, but at least it wouldn’t impact the design of your class.
Bad Practices
These patterns are aimed at surmounting the following problems:
- The class can’t be instantiated in a unit test environment
- Break Out Method Object
- Expose Static Method
- Parameterize Constructor
- Pull Up Feature
- A bad dependency is hard to mock or fake (because it is final, or because creating a subclass or mock is an unusual burden)
- Adapt Parameter
- Extract Implementer
- Extract Interface
- There is no “cut point” at which a bad dependency can be substituted by a mock or fake
- Extract and Override Call
- Extract and Override Factory Method
- Introduce Instance Delegator
- Introduce Static Setter
- Parameterize Constructor
- Parameterize Method
- Replace Global Reference with Getter
- Supersede Instance Variable
- There is a section of code that is untestable for some reason (it violates one of the principles of good unit tests)
- Subclass and Override Method
In other words:
- Creating classes that can’t be instantiated outside their “Integration” environment is a Bad Thing.
- Creating classes that are hard to mock or fake is a Bad Thing.
- Instantiating or using objects or static methods without any way to substitute them is a Bad Thing
- Doing things in your code which violate the basic principles of unit tests, and can’t be overridden in unit tests is a Bad Thing
As far as definitions of bad practices go, this list is pretty wishy-washy and hard to use as a litmus test for your code, especially if you’re trying to do it automatically, like I’d like to for the Legaciness metric. It looks like some more analysis and definition is in order. I’ll start with Bad Practice #4 by talking about something easy and well-known:
Principles of Unit Testing
Ok, there are tons of articles on this subject, so I won’t go into details. Tests are supposed to be maintainable, easy to run and trustworthy. Some consequences of this that relate to our Bad Practices:
- They shouldn’t affect state of system (tests should be self-contained, and be run in any order)
- The test should rely on as little code as possible (preferably only the single class/method that concerns you
- The outcome should be determinate (it shouldn’t depend on environment conditions, race conditions, etc.)
- It should run as fast as possible (over a second is considered too long, I think Michael Feathers sets 100ms as a ceiling)
So, for BP#4, we get the notion that we shouldn’t do things in our code like:
- Set global variables (e.g. change default Locale or Timezone, put things on the JNDI or the ApplicationContext, change environment variables, etc.)
- Have big ol’ honkin’ methods, or call methods like that without having a “cut point” (place to substitute and fake it)
- Use random numbers, spawn competing threads, base our results on the current time, etc.
- Call Thread.sleep() for long periods (as in the example I gave for “Subclass and Override Method”)
That’s a pretty huge constraint, so what the legacy patterns really tell us is that we shouldn’t do those things UNLESS WE HAVE A CUT POINT for them.
Bad Dependencies
Many of the other patterns related to having a bad dependency somewhere. “Bad” in this case doesn’t just mean “not mockable”. It also means that the dependency would violate one of the principles of unit testing. What sorts of dependencies are those? As I mentioned before, a instantiating a simple data class with only getters and setters using an explicit “new” isn’t going to do you any harm. Nor will a static call to Integer.valueOf(). But others aren’t so innocent. Examples might include:
- Database calls
- File system I/O
- JNDI dependencies
- EJB Container dependencies
- Global variables
… and so on. Note that there are some things that make it IMPOSSIBLE to test, and others that just add work.
Conclusion
This isn’t actually a conclusion… more like a beginning. The real question here is how this can be turned into a “Legaciness” metric. As I mentioned, I think It would be a useful compliment to code coverage and complexity reports. For the code that doesn’t have unit tests, how hard would it be to wedge some in there? But it appears that it won’t be trivial to turn this into a useful metric. I’ll explore these “Bad Practices” some more in future posts.
Posted by Timothy High 
