Wednesday, May 02, 2007

Unit Tests - Another Great Tool for Refactoring

In my last post on unit testing, I had talked about some techniques on how to make your classes unit test friendly. Unit testing is the bedrock of writing good software - never compromise on writing unit tests for the next class that you design. Unit testing is about testing classes in isolation - get them out of the natural environment and take them for a ride. That way unit tests give you a feedback about the correctness of the behavior of the *unit* as a whole. The other aspect of unit tests which I find extremely useful while designing classes is the support for refactoring. Unit tests are great vehicles for refactoring object collaborations - thus there is a 2-way feedback loop between class design and its unit test design. And once you have cycled through rounds of iteration in this feedback loop, you can expect to have stabilized your abstractions and collaborations.

Here's an example ..


class Payroll {
  // ..
  // ..

  void calculateSalary(Employee emp) {
    BigDecimal charge = emp.getSalary();
    // calculate total salary including taxes
  }
}



When I look at the possible unit test for the class Payroll, the first thing that strikes me is the argument Employee in calculateSalary(). The first attempt tries to instantiate an Employee and invoke the method calculateSalary() from the test case :


class PayrollTest extends TestCase {
  // ..
  // ..

  public void testSalaryCalculation() {
    Employee emp = new Employee(..);
    // ..
  }
}



A Mockery of sorts ..

Instantiating an Employee object brings into action lots of other machineries like the database layer, the ORM layer etc., which we already know should be best kept away from the unit test harness. Think Mocks ..

When you need to mock a concrete class, take a second look, if necessary take a third look as well. Mock roles, not objects and roles are best specified through interfaces. In the above example, we are really exploiting the fact that an employee is salaried and has a basic salary component for being employed in a company. In this method calculateSalary(), we are not concerned with the other details of the Employee class, which may be used in other context within the software.

Extract Interface

Now I have a new interface Salaried as :


interface Salaried {
  BigDecimal getBasicSalary();
}



and ..


public class Employee implements Salaried {
  // ..
}



The method calculateSalary() now takes an interface Salaried instead of the concrete class Employee - and the mock becomes idiomatic in the test case (I am using EasyMock as the mocking framework) :


class Payroll {
  // ..
  // ..

  public void calculateSalary(Salaried emp) {
    // ..
  }
}

class PayrollTest extends TestCase {
  // ..
  // ..

  public void testSalaryCalculation() {
    IBillable mock = createMock(Salaried .class);
    expect(mock.getBasicSalary()).andReturn(BigDecimal.valueOf(7000));
    replay(mock);
    // ..
  }
}




Thus unit testing helped me use the Extract Interface principle and improve the collaboration between my classes. Notice how the class Payroll now collaborates with a more generic role Salaried instead of a concrete class Employee. Tomorrow if the company decides to employ contractors with different salary computation, we will have a separate abstraction :


public class Contractor implements Salaried {
  // ..
  // ..
  BigDecimal getBasicSalary() {
    // .. different implementation than Employee
  }
  // ..
}



but the class Payroll remains unaffected, since it now collaborates with Salaried and NOT any concrete implementation.

6 comments:

Steve Freeman said...

Nice post. We've recently written up why we avoid mocking concrete classes

Sabarish Sasidharan said...

But then you really didn't have to use EasyMock to determine that you could be using a Salaried interface. IMO a self review could have achieved the same result.

Unknown said...

[for sabarish :] Very true, writing unit tests is one of the means of *self review* of the code that I have written. And using *mocks* is one way to streamline collaborations. And EasyMock is one of the most widely used mocking frameworks.

PierreR said...

You might end up with a profusion of interfaces if you apply this principle all along. Some interfaces might be quite specific to single methods.

Is this a problem ? A valid concern ? Does it always go along with the data abstraction principle ?

Unknown said...

[for pierre :]
If a concrete class plays multiple roles, then it is always recommended to publish the roles explicitly through interface driven modeling. In the example, one of the roles that the Employee abstraction was playing is that of a Salaried entity. And the method calculateSalary() of class Payroll is *only* concerned with the Salaried role of an Employee. Hence having the class Employee implement the Salaried interface is a valid "Extract Interface" refactoring. Initially I overlooked the possibility of having Salaried as a separate role of the entity. It was only during unit testing when I needed to mock a concrete class, did I realize that it could be a smell - and a refactoring was just around the corner.

PierreR said...

I don't know. Maybe I would just start by passing a BigDecimal to calculateSalary. I will create the interface as soon as I feel the need for it (a second parameter is needed, another data type (than Employee) need to "expose" salary). It is difficult to say because the context of the example is not so detailed.

What do you think ?