Anagram Code Kata Part 3 – Use Common Sense

This post is part of a series on coding Kata, BDD, MSpec, and SOLID principles.  Feel free to visit the above link which points to the introductory post, also containing an index of all posts in the series.

In this post, we will discuss how far to take the idea of creating mockable contracts via interfaces for testing purposes and to keep responsibilities modular.  As I solicited feedback from commenters, I was getting hung up on applying this concept to every object and dependency.  We’ll talk about how I’m now thinking that overly strict adherence to the practice can drive you nuts and decrease productivity.  It is meant to help guide your design process along rather than hinder it.  We’ll finish this post off with one of our two dependencies implemented in an extremely simple manner.

IFileParser Dependency

When we left off last time, we had defined two separate responsibilities that AnagramsFinder was going to push down into its dependencies.  As I started to write a specification test for the newly created NewlineFileParser, I realized it would be difficult to stub based on providing a file path to a text file as the only parameter.  I began having thoughts that maybe I could break up another pair of dependencies for file parsing, namely obtaining a file stream (or file contents string) from a path and then parsing the file contents into a list of multiple lines.  But then again, my first of the two dependencies is still dependent on the physical file system and I cannot get away from this unless I somehow mock up an in-memory file system.  This began to feel silly to me as I was hoping I would just use the built-in .NET APIs for file systems and string parsing.

After emailing Dave and Esteban asking for guidance, the revelation I received via these two fine gentlemen is that file parsing is not an essential purpose of this application nor exercise (at least not yet; we won’t be focusing on performance unless it appears to be grossly deficient as we go along).  What we could do is write integration tests instead, where we have a set of simple test files sitting in a folder that we run our NewlineFileParser against to ensure it working as we expect.  But I don’t think I’m even going to do this at this point.  I really do want to get to the core of the application.  Plus, I feel certain that I can trust the implementation of File.ReadAllLines() from the .NET Framework.

Unless it needs to be changed further down the road, the following code implementation will likely be sufficient for the remainder of this coding exercise:

public class NewlineFileParser : IFileParser
{
    public IEnumerable<string> ExtractWordListFromFile(string filePath)
    {
        return File.ReadAllLines(filePath);
    }
}

I feel great about this because I haven’t gone insane trying to mock and test everything in sight, nor have I broken up every single responsibility in such small chunks that it becomes tedious.  Instead, I have broken up the two core responsibilities of AnagramsFinder which means I can change and test the two dependencies independent of each other and from their manager class.  Even though I haven’t written a ton of code yet, I feel the tests specs have driven my design into a cohesive, decoupled design that should prove flexible if requirements change or if I encounter friction later on and need to rethink my design.  A flexible architecture for the application is being flushed out based on a “top-to-bottom” design view of the requirements.

Create Stubs or Assert Methods were Called

Tobias Walter left a comment on my last post with some suggestions that I would like to get clarification on and discuss further with my growing audience of three now.  He compared some of my implementation to Dave’s implementation on his Favour test driving logic over data post, but the comparison was lost on me.  It seemed Tobias was saying that I had gone too far breaking up the responsibilities into such small dependencies.  I am unsure what his argument was, but as far as I could tell both Dave and I broke up a managing class into two separate responsibilities so that the more crucial of the two could be tested independently.  Also in doing so, we both created stubs that were given specific dummy responses to method calls using Rhino.Mocks.  One more comment I would like to say is that I broke the file parsing responsibility away into its own dependency more because of the need to isolate the anagrams grouping than because file parsing is of itself a difficult task.  If Tobias or anyone else could try to clarify his argument, that would really help me understand the merits of his suggestions.

Another suggestion that Tobias brought up that I would like to petition feedback on is to use the mocking feature of testing whether a method was called instead of creating a stub programmed with a dummy response.  I have to admit, I’m not sure when to use one method over another.  I guess I’ve felt in the past that testing whether a method was called seems silly, since it seems odd to have your test know the implementation and inner method calls of the subject under test.  But I am now realizing that my stubs appear to be doing the same thing, but with dummy input and output programmed in.  If you have an opinion or experience on this subject, please leave some feedback in the comments of this post.

What’s Next?

In my next post I hope to summarize this mocking debate brought up above regarding two differing methods of verifying my manager class is correctly managing its dependencies.  I also hope to begin driving out the design and implementation of the AnagramGrouper dependency via behavior specification tests.  Thanks and any feedback is greatly appreciated.

Comments

David
Hi Mike,

In terms of the stubbing vs. asserting a method was called, the team I'm on has found we rarely AssertWasCalled any more. Doing so explicitly couples our tests/specs to the implementation, whereas just stubbing calls in a SetUp seems easier to change without altering the fundamental assertions in our tests.

The only time we use AssertWasCalled is for void methods to a service or similar where there is no observable side-effect from a unit perspective.

YMMV. :)

Regards,
David
Tobias Walter
Hi Mike,

great you continued this series.

I think I messed things up and wrote incorrect things. In the meantime I managed to do this kata again and now I agree with you. Just forget my advice to mock it - this was wrong.

However, I and my coworkers question if it's worth to write 30 lines of code to test a 2-lines-method? I think for this kata it will be enough to do some acceptance tests here.