Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

If I have a function in my code that goes like:

class Employee{

    public string calculateTax(string name, int salary)
    {
        switch (name)
        {
            case "Chris":
                doSomething($salary);
            case "David":
                doSomethingDifferent($salary);
            case "Scott":
               doOtherThing($salary);               
       }
}

Normally I would refactor this to use Ploymorphism using a factory class and strategy pattern:

public string calculateTax(string name)
{
    InameHandler nameHandler = NameHandlerFactory::getHandler(name);
    nameHandler->calculateTax($salary);
}

Now if I were using TDD then I would have some tests that work on the original calculateTax() before refactoring.

ex:

calculateTax_givenChrisSalaryBelowThreshold_Expect111(){}    
calculateTax_givenChrisSalaryAboveThreshold_Expect111(){}

calculateTax_givenDavidSalaryBelowThreshold_Expect222(){}   
calculateTax_givenDavidSalaryAboveThreshold_Expect222(){} 

calculateTax_givenScottSalaryBelowThreshold_Expect333(){}
calculateTax_givenScottSalaryAboveThreshold_Expect333(){}

After refactoring I'll have a Factory class NameHandlerFactory and at least 3 implementation of InameHandler.

How should I proceed to refactor my tests? Should I delete the unit test for claculateTax() from EmployeeTests and create a Test class for each implementation of InameHandler?

Should I test the Factory class too?

share|improve this question

4 Answers 4

up vote 6 down vote accepted

The old tests are just fine for verifying that calculateTax still works as it should. However, you don't need many test cases for this, only 3 (or maybe some more, if you want to test error handling too, using unexpected values of name).

Each of the individual cases (at the moment implemented in doSomething et al.) must have their own set of tests too, which test the inner details and special cases related to each implementation. In the new setup these tests could / should be converted into direct tests on the respective Strategy class.

I prefer to remove old unit tests only if the code they exercise, and the functionality it implements, completely ceases to exist. Otherwise, the knowledge encoded into these tests is still relevant, only the tests need to be refactored themselves.

Update

There may be some duplication between the tests of calculateTax (let's call them high level tests) and the tests for the individual calculation strategies (low level tests) - it depends on your implementation.

I guess the original implementation of your tests asserts the result of the specific tax calculation, implicitly verifying that the specific calculation strategy was used to produce it. If you keep this schema, you will have duplication indeed. However, as @Kristof hinted, you may implement the high level tests using mocks too, to verify only that the right kind of (mock) strategy was selected and invoked by calculateTax. In this case there will be no duplication between high and low level tests.

So if refactoring the affected tests isn't too costly, I would prefer the latter approach. However, in real life, when doing some massive refactoring, I do tolerate a small amount of test code duplication if it saves me enough time :-)

Should I test the Factory class too?

Again, it depends. Note that the tests of calculateTax effectively test the factory. So if the factory code is a trivial switch block like your code above, these tests may be all you need. But if the factory does some more tricky things, you may want to dedicate some tests specifically for it. It all boils down to how much tests you need to be confident that the code in question really works. If, upon reading the code - or analysing code coverage data - you see untested execution paths, dedicate some more tests to exercise these. Then repeat this until you are fully confident in your code.

share|improve this answer
    
I modified the code a little to make it closer to my actual practical code. Now a second input salary to the function calculateTax() was added. This way I think I'll be duplicating the test code for the original function and the 3 implementations of the strategy class. –  Songo Aug 27 '12 at 10:28
    
@Songo, please see my update. –  Péter Török Aug 27 '12 at 11:58

I'll start by saying that I'm no expert on TDD or unit testing, but here's how I would test this (I'll use pseudo-like code):

CalculateTaxDelegatesToNameHandler()
{
    INameHandlerFactory fakeNameHandlerFactory = Fake(INameHandlerFactory);
    INameHandler fakeNameHandler = Fake(INameHandler);

    A.Call.To(fakeNameHandlerFactory.getHandler("John")).Returns(fakeNameHandler);

    Employee employee = new Employee(fakeNameHandlerFactory);
    employee.CalculateTax("John");

    Assert.That.WasCalled(fakeNameHandler.calculateTax());
}

So I'd test that the calculateTax() method of employee class correctly asks its NameHandlerFactory for a NameHandler and then calls the calculateTax() method of the returned NameHandler.

share|improve this answer
    
hmmmm so you mean I should make the test a behavioral test instead (testing that certain functions were called) and make the value assertions on the delegated classes? –  Songo Aug 27 '12 at 10:15
    
Yes, that's what I would do. I would indeed write seperate tests for the NameHandlerFactory and the NameHandler. When you have those, there's no reason to test their functionality again in the Employee.calculateTax() method. That way you don't need to add extra Employee-tests when you introduce a new NameHandler. –  Kristof Claes Aug 27 '12 at 11:22

Before writing any code, I would start with a test for a Factory. Mocking the stuff I need I would force myself to think about the implementations and usecases.

Than I would implement a Factory and continue with a test for each implementation and finally the implementations themselves for those tests.

Finally I would remove the old tests.

share|improve this answer

You're taking one class (employee that does everything) and making 3 groups of classes: the factory, the employee (that just contains a strategy) and the strategies.

So make 3 groups of tests:

  1. Test the factory in isolation. Does it handle inputs correctly. What happens when you pass in an unknown?
  2. Test the employee in isolation. Can you set an arbitrary strategy and it works as you expect? What happens if there is no strategy or factory set? (if that's possible in code)
  3. Test the strategies in isolation. Does each perform the strategy you expect? Do they handle odd boundary inputs in a consistent manner?

You can of course make automated tests for the whole shebang, but these are now more like integration tests and should be treated as such.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.