We’ve all been there and done it – written some code that at the time seemed great but you look back on it and think “why!”. I’m hoping to save you from at least one of those situations today with a brutal dissection of the Template pattern. Seductively, it offers you a quick and easy path to reusing code. Unfortunately in doing so, it makes your code incredibly difficult to test.
Let’s pretend you are in a situation like this. You have to parse several files, all in different formats e.g. XML, CSV, TXT etc.. All of them should have some common functionality like error handling, logging, business logic etc. and share the same directory that they read from. Each one should just do the specific bit about reading from that type of file and reuse the boilerplate stuff we just mentioned. So, because you’re a cool OO kind of guy, you think of inheritance and code reuse, and end up with a hierarchy like this: –
IsValidFile() and GetFileContents() are abstract and implemented by the concrete implementations; all the “driver” business logic is held in the abstract FileReader class, whose ReadFiles() method looks something like this: –
I’ve highlighted the calls to the protected abstract methods that the derived classes must implement. It’s beautiful. We can treat all of these Reader classes polymorphically in some main method, and it’ll just work!
What went wrong?
Congratulations – you’ve now been trapped by Template. When you try to test out your derived classes, even though you’ve taken the precaution of mocking out DataAccess, Logger and FileAccess, you’ll immediately come up against some problems like these: –
- How do you test your file readers when you can’t directly call the methods under test (the ones in red above) – they’re all protected?
- How do you test your FileReader’s business logic out when it’s closely coupled to some implementation that is irrelevant to the FileReader code?
And please don’t say “use reflection” or “make the methods public or internal”. We’ve gone to the trouble of making an OO class hierarchy, so let’s do it properly.
Your tests on the derived classes will all suffer from the following problems: –
- Excessive and irrelevant setup: You’ll need to call the public GetFiles() method to test out either of the two protected method. Therefore you’ll need to mock out things like IDal and ILogger even though they aren’t being used by the class under test.
- Difficult to write: Your tests will have to pass any business logic that flows into your protected methods e.g. in our case, whatever happens in the FileIsInCorrectDateRange() method will have to pass in order to get into the GetFileContents() call. Even worse, to test out GetFileContents(), we have to also mock out IsValidFile() on the same class because it’s called immediately before.
- Duplication of code: Exactly what you wanted to avoid in the first place. Unfortunately, your tests for your three derived classes will have exactly the same problems. The only solution that you might come up with is to template your tests classes as well. Don’t do it.
Writing the tests
Here’s an example of what your attempts at writing test methods might look like for testing out the IsValidFile() method of CSVFileReader class, which for arguments sake lets assume just looks at whether the extension of the filename is “.csv”.
OK. Can’t do that then… let’s try naming it like this: –
Still doesn’t really make sense. Nonetheless, let’s press on and actually write the test method, using a mocking framework where required (in the example below I’m using Typemock’s awesome Isolator framework, but you could just as easily use Rhino or Moq): –
Compare the size of this single test method with the size of the original, production code. Hideous. Absolutely awful. And this is a simple bit of logic. Imagine if there were other calls involved as well e.g. logging etc..
- First we create our CsvFileReader, plus faking all the dependencies that it has.
- Then we fake the result of FileAccess.GetFiles() to return a single filename for the foreach loop.
- Next, we fake the result of GetDateOfFile() so that the second condition of the if statement passes.
- Next, because we will call the other method on the CsvFileReader (GetFileContents), we need to mock another method so that that call passes
- Finally, we do the act.
- Lastly, we assert that the DAL was called.
Notice that basically nowhere in this test do we talk about IsValidFile(). We’re talking about mocking stuff here and there, file access, data access etc. It’s impossible to see what we’re really trying to test.
All this just to test a single method that takes in a string and returns a Boolean?
In my next post, I’ll talk about how we can redesign our class hierarchy using a different design pattern to Template which will give us much more flexibility not to mention much simpler tests.