Legaciness revealed!

October 1, 2008

It looks like my neverending quest for a Legaciness metric has already come to a satisfyingly swift end! I was just browsing some blogs on agile techniques when I came across a post about fixing bad developer habits on a blog called Agile Tips. Waaaaay down in the footnotes is a link to a handy little tool in the Google Code site called the Testability Explorer.

I couldn’t believe my eyes when I saw it. I felt like Francisco Orellana stumbling across El Dorado. Could this really be the fabled Legaciness metric after all? I have to investigate more, but what I’ve read so far seems to be exactly what I’ve been searching for: a tool designed to calculate the difficulty of testing Java code based on its static dependencies! And from what I’ve read so far, it looks like I was on the right track with the algorithm:

  1. Each class attribute has to be considered by how “injectable” it is (their term, and I like it)
  2. “Global” variables are considered bad if they’re accessible, but not injectable
  3. There’s a transient penalty for methods which call methods which are not injectable (the “inheritance” penalty that I talked about)

They don’t directly address a number of concerns I have, such as:

  1. System calls that make testing difficult (e.g. I/O calls)
  2. Calls to “new” within a method
  3. The ability to “inject” something using the Legacy Code Patterns like “Extract and Override Factory Method” (if the class itself is considered injectable in a method, at least that method won’t get a penalty)

I’m guessing that some of these concerns, like the calls to “new”, would be covered by the transitivity of the complexity scores. I also like the touch that they’ve included cyclomatic complexity into the mix, although I don’t have a feel yet for if that would outweigh the injectability problems – my guess is no. I also don’t know if they take into account 3rd-party libraries. All in all, it looks like their metric is pretty straightforward, easy to understand and useful. I like the simplicity of their approach.

All that’s left now is to try it out! I’ll give it a shot tomorrow and see what it thinks about a certain 550-line file-reading Excel-interpreting monster of a method that’s been keeping me up at night…

Advertisements

Legacy Patterns Decision Tree

August 27, 2008

Way back when I first read the Michael Feathers book on “Working Effectively with Legacy Code”, I decided to write up a quick description of the patterns in the book for my coworkers to look up whenever they need them (basically, when trying to write unit tests for big, monolithic “legacy” classes).

These patterns are extremely intuitive once you’ve gone over them a few times. They’re really just obvious techniques for working around some constraints with as little risk and impact as possible. However, to reduce the learning curve, I created the decision tree below. I actually don’t think it’s very useful in practice, but creating it was a great way for ME to fully understand these patterns and their relationships. Now, I pass it on to you. My hope is that any of you developers out there may learn something from having a look at it. And, who knows, maybe there’s some insight here that can be used in my quixotic quest for the Legaciness metric.

Legacy Code Patterns Decision Tree

Legacy Code Patterns Decision Tree

Note that some of the leaf nodes recommend the use of mock objects, which are not part of the legacy code patterns themselves. I recommend the use of EasyMock, which is the library I’m using, but you can use whatever you prefer. If you can use fake objects, I find that to be generally easier than, and thus preferable to using mock objects.


Legacy Code Bad Practices

August 15, 2008

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:

  1. The class can’t be instantiated in a unit test environment
    • Break Out Method Object
    • Expose Static Method
    • Parameterize Constructor
    • Pull Up Feature
  2. 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
  3. 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
  4. 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:

  1. Creating classes that can’t be instantiated outside their “Integration” environment is a Bad Thing.
  2. Creating classes that are hard to mock or fake is a Bad Thing.
  3. Instantiating or using objects or static methods without any way to substitute them is a Bad Thing
  4. 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.


Legaciness

July 29, 2008

I’ve been looking at software metrics recently as a way to track how we’re doing with our code quality. Of course, not all metrics are equal, and they are often about as helpful and accurate as a 7-day weather report… Neal Ford from Thoughtworks recently gave a presentation on his take of software metrics, which he said was something like “reading tea leaves”. A single metric on its own, out of context, may not tell you anything at all (what does a cyclomatic complexity rating of 25 mean, anyways? Is that good or bad? Hell, this rating doesn’t even have UNITS!). It’s only when grouping them together with other metrics and comparing them relatively against other pieces of code that you start to get some sort of idea.

I consider the CRAP4J crappiness rating to be something of an exception. The people who came up with it had a great idea: what if we take two metrics that offset one another, compare them and boil it down into a new value? In other words, what if we do some of the tea reading for you? The two metrics they are using are McCabe’s cyclomatic complexity rating, which gives you a measure more or less of how many different paths there are through a given method (the more paths, the more complex), and a code coverage measure of their own something like Cobertura which can measure the number of paths that are covered by unit tests. This makes perfect sense: if you have a lot of paths, that’s bad, but if you’re testing all the paths, that should be perfectly OK. If you were trying to read these leaves on your own, you would have to paw through all the parts of code that have the highest complexity rating, then try to look up a corresponding report on test coverage…

So my hat’s off to the guys that did CRAP4J.

I’ve also been considering coming up with a metric of my own that I call “Legaciness”. I get my inspiration from Michael Feather’s fantastic book “Working Effectively with Legacy Code”. In his book, he defines legacy code as code which isn’t covered by unit tests. That in itself is a great definition. But the focus of my metric wouldn’t be on the test coverage itself (which we can already measure). The majority of the book focuses on all the tricky issues that can make “legacy” code hard to test in the first place. He lists all sorts of bad practices, like static method calls, doing Bad Things in the constructor, and so on. Then the second half of the book enumerates a series of patterns that can be used in Java, C++ and other languages to safely and slowly refactor your code JUST ENOUGH to be able to slip them into some unit tests, after which you should be able to redesign your code as you wish.

The key here that I’d like to measure is exactly HOW HARD would it be to get some unit tests around your code. In other words, what would be the amount of effort necessary to refactor a class for unit testing; how many time would the Legacy Patterns have to be invoked in order to get 100% test coverage for that class? This measure is what I would call “Legaciness”.

I’ve gone through a lot of the thinking process as to how this sort of thing would be measured, and a number complications have crept up:

  1. Not all static calls are an obstacle to unit testing. For example, a utility method that converts a String to all caps wouldn’t get in the way. So, the metric ideally should take that sort of thing into account.
  2. Explicit calls to “new” are generally a bad thing, since there’s no way to substitute the object being instantiated with a mock object. But, again, if the class being instantiated is a simple Data Transfer Object (DTO) with a bunch of getters and setters, what’s the problem?
  3. Even if a call to “new” is a problem, one of the patterns to remove the obstacle is to simply extract the line of code into a protected method which can then be overridden in a test class. So the metric would have to somehow take into account when one of the legacy code patterns has been implemented to surmount these obstacles.
  4. The Legaciness score would have to be crafty enough to take into account certain levels of indirection. For example, it might be slightly bad that a method instantiates a class via “new”. But it should be considered MUCH WORSE if the class it instantiates also has a really bad Legaciness rating via, say, direct calls to I/O methods and threading and so on and so forth. So, there seems to be some need for an “inheritance” of Legaciness through these dependencies.
  5. When you get down to it, a lot of these bad dependencies are due to I/O calls, threading, container dependencies and so on. These dependencies are very likely outside your own code base (for example, in the base Java APIs). The only realistic measure I could think of is to come up with a base Legaciness score for all known API methods. This seems like more work than it’s worth (actually, Neal Ford mentioned to me that someone did this very thing with Ruby: assigned a score to each base API method for some nefarious metric of their own). Also, I don’t know if the same would be needed for other 3rd-party libraries, or if it would be enough to cover just the java.* and javax.* libraries, and then analyze their Legaciness based on their use of the Java APIs.

It seems to me that Legaciness would be another very useful metric to have, much like CRAP4J. If the latter tells you where your code is complex and untested, the former could tell you just how hard it would be to to give it the test coverage it needs. I think this metric would also be useful to help educate developers as to some of the same best practices that TDD is supposed to encourage.

Unfortunately, I don’t have the time to work out the details of such a metric. If anyone out there’s interested, or has any ideas, let me know!