Friday, March 30, 2007

Making Classes Unit-Testable

I had been working on the code review of one of our Java projects, when the following snippet struck me as a definite smell in one of the POJOs :


class TradeValueCalculator {
  // ..
  // ..

  public BigDecimal calculateTradeValue(final Trade trade, ..) {
    // ..
    BigDecimal tax = TradeUtils.calculateTax(trade);
    BigDecimal commission = TradeUtils.calculateCommission(trade);
    // .. other business logic to compute net value
  }
  // ..
  // ..
}



What is the problem with the above two innocuous looking Java lines of code ? The answer is very simple - Unit Testability of the POJO class TradeValueCalculator ! Yes, this post is about unit testability and some tips that we can follow to design classes that can be easily unit tested. I encountered many of these problems while doing code review of a live Java project in recent times.

Avoid Statics

When it comes to testability, statics are definitely not your friends. In the above code snippet, the class TradeValueCalculator depends on the implementation of the static methods like TradeUtils.calculateTax(..) and TradeUtils.calculateCommission(..). Any change in these static methods can lead to failures of unit tests of class TradeValueCalculator. Hence statics introduce unwanted coupling between classes, thereby violating the principle of easy unit-testability of POJOs. Avoid them, if you can, and use standard design idioms like composition-with-dependency-injection instead. And while using composition with service components, make sure they are powered by interfaces. Interfaces provide the right level of abstraction for multiple implementations and are much easier to mock while testing. Let us refactor the above snippet to compose using service components for calculating tax and commission :


class TradeValueCalculator {

  // .. to be dependency injected
  private ITaxCalculator taxCalculator;
  private ICommissionCalculator commissionCalculator;

  // ..
  // ..

  public BigDecimal calculateTradeValue(final Trade trade, ..) {
    // ..
    BigDecimal tax = taxCalculator.calculateTax(trade);
    BigDecimal commission = commissionCalculator.calculateCommission(trade);
    // .. other business logic to compute net value
  }
  // ..
  // ..
}

interface ITaxCalculator {
  BigDecimal calculateTax(..);
}

interface ICommissionCalculator {
  BigDecimal calculateCommission(..);
}



We can then have concrete instances of these service contracts and inject them into the POJO TradeValueCalculator :


class DefaultTaxCalculator implements ITaxCalculator {
  // ..
}

class DefaultCommissionCalculator implements ICommissionCalculator {
  // ..
}



Using standard IoC containers like Guice or Spring, we can inject concrete implementations into our POJO non-invasively through configuration code. In Guice we can define Modules that bind interfaces to concrete implementations and use Java 5 annotation to inject those bindings in appropriate places.



// define module to configure bindings
class TradeModule extends AbstractModule {

  @Override
  protected void configure() {
  bind(ITaxCalculator .class)
      .to(DefaultTaxCalculator .class)
      .in(Scopes.SINGLETON);

    bind(ICommissionCalculator .class)
      .to(DefaultCommissionCalculator .class)
      .in(Scopes.SINGLETON);
  }
}



and then inject ..


class TradeValueCalculator {

  // ..
  @Inject private ITaxCalculator taxCalculator;
  @Inject private ICommissionCalculator commissionCalculator;

  // ..
  // ..
}



How does this improve testability of our class TradeValueCalculator ?

Just replace the defined Module by another one for unit testing :


// define module to configure bindings
class TestTradeModule extends AbstractModule {

  @Override
  protected void configure() {
    bind(ITaxCalculator .class)
      .to(MockTaxCalculator .class)
      .in(Scopes.SINGLETON);

    bind(ICommissionCalculator .class)
      .to(MockCommissionCalculator .class)
      .in(Scopes.SINGLETON);
  }
}



What we have done just now is mocked out the service interfaces for tax and commission calculation. And that too without a single line of code being changed in the actual class! TradeValueCalculator can now be unit-tested without having any dependency on other classes.

Extreme Encapsulation

I have come across many abuses of FluentInterfaces, where developers use chained method invocations involving multiple classes. Take this example from this Mock Objects paper, which discusses this same problem :

dog.getBody().getTail().wag();

The problem here is that the main class Dog is indirectly coupled with multiple classes, thereby violating the Law of Demeter and making it totally unsuitable for unit testing. The situation is typically called "The Train Wreck" and has been discussed extensively in the said paper. The takeway from this situation is to minimize coupling with neighbouring classes - couple only with the class directly associated with you. Think in terms of abstracting the behavior *only* with respect to the class with which you collaborate directly - leave implementation of the rest of the behavior to the latter.

Privates also need to be Unit-Tested

There is a school of thought which espouses the policy that *only* public api s need to be unit-tested. This is not true - I firmly believe that all your methods and behaviors need unit testing. Strive to achieve the maximum coverage of unit testing in your classes. Roy Osherove thinks that we may have to bend some of the rules of pure OOD to make our design implementations more testable e.g. by exposing or replacing private instances of objects using interfaces, injection patterns, public setters etc. Or by discouraging default sealing of classes allowing overriding in unit tests. Or by allowing singletons to be replaced in tests to break dependencies. I think, I agree to many of these policies.

Fortunately Java provides a useful access specifier that comes in handy here - the package private scope of access. Instead of making your implementation members *private*, make them *package private* and implement unit test classes in the same package. Doing this, you do not expose the private parts to the public, while allowing access to all unit test classes. Crazy Bob has more details on this. Another useful trick to this may be usage of AOP. As part of unit test classes, you can introduce additional getters through AOP to access the implementation artifacts of your class. This can be done through inter-type declarations, and the test classes can access all private data at gay abandon.

Look out for Instantiations

There are many cases where the class that is being unit tested needs to create / instantiate objects of the collaborating class. e.g.


class TradeController {

  // ..
  // ..

  public void doTrade(TradeDTO dto, ..) {
    Trade trade = new Trade(dto);
    // .. logic for trade
  }
  // ..
}



Increase the testability of the class TradeController by separating out all creation into appropriate factory methods. These methods can then be overridden in test cases to inject creation of Mock objects.


class TradeController {
  TradeDTO dto;

  // ..
  // ..

  public void doTrade() {
    Trade trade = createTrade(dto);
    // .. logic for trade
  }
  // ..

  protected Trade createTrade(TradeDTO dto) {
    return new Trade(dto);
  }
}



and create MockTrade in test cases ..


class TradeControllerTest extends TestCase {

  // ..

  public void testTradeController(..) {
    TradeController tc = new TradeController() {
      protected Trade createTrade(TradeDTO dto) {
        return new MockTrade(dto);
      }
    }
    tc.doTrade();
  }
}



The Factory Method pattern proves quite helpful in such circumstances. However, there are some design patterns like The Abstract Factory, which can potentially introduce unwanted coupling between classes, thereby making them difficult to unit-test. Most of the design patterns in GOF are built on composition - try implementing them using Interfaces in Java, so that they can be easily mocked out. Another difficult pattern is the Singleton - I usually employ the IoC container to manage and unit-test classes that collaborate with Singletons. Apart from static methods, which I have already mentioned above, static members are also problematic cases for unit testing. In many applicatiosn they are used for caching (e.g. ORMs) - hence an obvious problem child for unit testing.

8 comments:

Ricky Clarkson said...

A broken static method can break a test just as easily as a broken non-static method can.

Static methods are simple to use. It's easy to read code that uses static methods. It's not so easy to mock them out, but perhaps AOP can help there too. In Scheme at least, you can rebind a function temporarily. It's a shame that Java's class model is so static.

In short, if you have to write harder-to-read code to make it testable, then there's something wrong with the language.

Unknown said...

[for ricky]:
The advantage of using non-static method is that the instance can always be mocked appropriately, more so using an IoC like Spring or Guice. For statics, mocking is difficult and makes the code tightly coupled to the class whose static method is used.

I agree with ur observations in Scheme (which can also be done in Ruby), where metaprogramming can make you reach very obscure corners of the programs. But again, my whole post was based on Java, where we need to deal with a static class model. And yes, AOP can always be used to do all sorts of magic.

Anonymous said...

We used AspectJ in last project to get static methods tested. In my opinion it was not too easy or convinient. Too much hassle to get simple tests working...

Anonymous said...

I think in first static example there is also other code smell. I would put these calculations that TradeUtils does in domain model and avoid this kind of utility classes.

Ricky Clarkson said...

I understand that instance-based mocking is easier, but there is the slight penalty of code being a bit harder to understand. Math.min(2,3) works, and we never feel the need to mock it, so why do we feel the need to mock out other static methods?

Tests that depend on functionality from more than one place is ok, and actually unavoidable. If you write tests for your static methods, then there's no reason not to use those static methods elsewhere.

Converting statics to instance methods? YAGNI.

Anonymous said...

Using static methods on many service methods will cause procedural program flow so I think this static vs. instance methods will set up base for application and its program flow.

For me this also means anemic domain model vs rich domain model.

Jeremy Weiskotten said...

Static methods are the equivalent of functional programming, which is fine if you are willing to give up the benefits of OO. They make sense in a lot of cases, for example utility methods like Math.abs, but as soon as they do anything more complicated, like fetch something from a database, it's time to refactor for a variety of reasons, not the least of which is testability. This is where the DAO pattern came from.

In previous projects the typical data access pattern involved static methods in the model for fetching objects from the database (or cache). This has obvious implications on unit testing that class and others that depend on that class. My favorite solution to the unit testing dilemma was to extract an interface to define a data access facade, implement it for production by delegating to the static methods, and mock it for unit testing.

This way existing code can still call the static methods directly, but over time legacy code that is being brought under test could be refactored to use the facade. Either a dependency injecting factory or a simple factory method that can be overridden in the test is all it takes to provide the appropriate facade implementation.

Adil said...

Thank you for this very nice article !