This is the wrap up of my second attempt at Roy Osherove's String Calculator kata. You can read the details at the original site, but the overall gist is a calculator that has an Add() method. This method takes a string input, adds the numbers in the string together, then returns the result.
In Part 1 of this series we looked at using a traditional TDD approach to the calculator kata. In Part 2 we attempted to use a variation of Behavioural Driven Development (BDD) . In this part we'll finish off our BDD example. So far our implementation handles empty strings, single numbers, and numbers delimited by a comma and/or a newline.
To accomplish this, we have a Calculator class that uses an INumberParser to parse numbers from the string, and then an IAdder to calculate the sum of these numbers.
The final part of the exercise (well, as far as we're going for this series) is to be able to parse expressions that specify a custom delimiter. So the input to our Add() method might be in the form //[delimiter]\n[numbers]. We also need to continue handling the case were there is no delimiter specified.
This post is pretty code heavy, so if you're viewing it in an RSS reader you might find it more readable on my site where the code has syntax highlighting.
Where should customer delimiter logic go?
This seems like a different concern to me -- we are no longer just parsing out numbers, we are parsing out a delimiter message, then parsing out numbers from the remaining string based on that. Where should we add this concern? Well our Calculator itself is looking pretty tight already:
public class Calculator {
private readonly INumberParser _numberParser;
private readonly IAdder _adder;
public Calculator(INumberParser numberParser, IAdder adder) {
_numberParser = numberParser;
_adder = adder;
}
public int Add(string expression) {
var numbers = _numberParser.GetNumbers(expression);
return _adder.Add(numbers);
}
}
This seems a really clear implementation of what we are trying to achieve: we are getting numbers from an expression and adding them. The current specification for this class also looks pretty neat. I don't really fancy mucking it up by shoehorning in this new concern at this level of abstraction.
Maybe we can push this down into the INumberParser? The intention behind the custom delimiter feature is to provide another way to parse numbers, so this sounds pretty reasonable. Let's try creating a CalculatorExpressionParser that implements INumberParser, which can parse out the delimiter and delegate to our original NumberParser. I'm unsure of exactly what this will look like at this stage (will we have a series of handlers selected using the Specification Pattern? Or decorate our current number parser?), but we'll let our tests/specification drive out the details.
Specifying our CalculatorExpressionParser
Let's start our CalculatorExpressionParserSpec and see where that leads us.
public class When_parsing_expression : ConcernFor<CalculatorExpressionParser> {
[Test]
public void Should_parse_numbers_using_specified_delimiter() {
Assert.That(result, Is.EquivalentTo(numbers));
}
protected override void Because() {
result = sut.GetNumbers(expression);
}
}
We still want the same basic INumberParser behaviour, so the spec isn't telling us much here. The context is where it is going to get a little scary. We want to parse out the delimiter from the expression, then remove the delimiter information from the expression and feed that into our standard NumberParser. Let's start with this:
protected override void Context() {
char[] delimiter = new[]{';'};
const string numbersExpression = "1;2;3;4;5";
expression = "//;\n1;2;3;4;5";
delimiterParser = MockRepository.GenerateStub<IDelimiterParser>();
delimiterParser
.Stub(x => x.Parse(expression))
.Return(new DelimitedNumbersExpression(numbersExpression, delimiter));
/*.... need to use this output somehow... */
}
We've now pushed down the responsibility for parsing out the delimiter and updating the expression to an IDelimiterParser. Because C# functions only support single return values, we'll need a new class to hold these two pieces of information which I'll call DelimitedNumbersExpression. This will just be a DTO, so we may as well code this up now:
public class DelimitedNumbersExpression {
public char Delimiter { get; private set; }
public string DelimitedNumbers { get; private set; }
public DelimitedNumbersExpression(char delimiter, string delimitedNumbers) {
Delimiter = delimiter;
DelimitedNumbers = delimitedNumbers;
}
}
Now we need to configure a NumberParser with this required delimiter, and give it our numbersExpression to parse. This is starting to sound like another responsibility, isn't it? Let's use a factory to create a configured NumberParser for us. We'll update the context like this:
protected override void Context() {
char[] delimiter = new[]{';'};
const string numbersExpression = "1;2;3;4;5";
expression = "//;\n1;2;3;4;5";
numbers = new [] {1, 2, 3, 4, 5};
delimiterParser = MockRepository.GenerateStub<IDelimiterParser>();
delimiterParser
.Stub(x => x.Parse(expression))
.Return(new DelimitedNumbersExpression(numbersExpression, delimiter));
numberParserFactory = MockRepository.GenerateStub<INumberParserFactory>();
numberParser = MockRepository.GenerateStub<INumberParser>();
numberParserFactory.Stub(x => x.CreateWithDelimiters(delimiter)).Return(numberParser);
numberParser.Stub(x => x.GetNumbers(numbersExpression)).Return(numbers);
}
protected override CalculatorExpressionParser CreateSubjectUnderTest() {
return new CalculatorExpressionParser(delimiterParser, numberParserFactory);
}
This is a bit scary, particularly if you haven't used a mocking framework before. :) We've given ourselves another dependency, an INumberParserFactory which has a CreateWithDelimiter() method. We'll then return a stubbed INumberParser, which in turn will process our numbersExpression and return our required result.
Notice how we are testing a lot of things implicitly here. We don't have a test called Should_create_number_parser_from_factory() or similar, although this is in fact tested indirectly via the context (i.e. if it were not setup in the context, our actual test would not pass!). This stops our tests from being too brittle -- we can change how the SUT does its job by modifying the context, while leaving most of the specification (the scenario name, test name, assertion, and the interface used in the Because() method) unchanged.
On the positive side, this is trivial for us to implement:
public class CalculatorExpressionParser : INumberParser {
private readonly IDelimiterParser _delimiterParser;
private readonly INumberParserFactory _numberParserFactory;
public CalculatorExpressionParser(IDelimiterParser delimiterParser, INumberParserFactory numberParserFactory) {
_delimiterParser = delimiterParser;
_numberParserFactory = numberParserFactory;
}
public IEnumerable<int> GetNumbers(string expression) {
var delimitedNumbersExpression = _delimiterParser.Parse(expression);
var numberParser = _numberParserFactory.CreateWithDelimiters(delimitedNumbersExpression.Delimiters);
return numberParser.GetNumbers(delimitedNumbersExpression.DelimitedNumbers);
}
}
I'm not 100% happy with this: that context is pretty mean and I think I have the names wrong, but the code is still flowing freely and seems fairly clean so let's press on.
Confession: I actually intially coded this with INumberParserFactory only accepting a single character delimiter, then refactored to support multiple delimiters for the default case of ',' and '\n' (this was a concious decision). I've omitted this step in consideration of your bandwidths costs and in the interest of finishing this post this year. :)
A delimiter parser
Our previous spec required the use of an IDelimiterParser. As I see it the DelimiterParser has to cope with two separate scenarios: when the expression contains a delimiter specifier, and when it doesn't. Let's start with the case where it actually needs to do some work.
public class When_parsing_expression_that_starts_with_a_delimiter_specifier : ConcernFor<DelimiterParser> {
private DelimitedNumbersExpression result;
private string expression;
private string delimitedNumbers;
private char delimiter;
[Test]
public void Should_return_specified_delimiter() {
Assert.That(result.Delimiters, Is.EquivalentTo(new[] {delimiter}));
}
[Test]
public void Should_return_delimited_numbers_without_delimiter_specifier() {
Assert.That(result.DelimitedNumbers, Is.EqualTo(delimitedNumbers));
}
protected override void Because() {
result = sut.Parse(expression);
}
protected override void Context() {
delimiter = ';';
delimitedNumbers = "1;2;3";
expression = "//" + delimiter + "\n" + delimitedNumbers;
}
protected override DelimiterParser CreateSubjectUnderTest()
{
return new DelimiterParser();
}
}
//DelimiterParser.cs
public DelimitedNumbersExpression Parse(string expression) {
var delimiter = expression[2];
var delimitedNumbers = expression.Substring(4);
return new DelimitedNumbersExpression(delimiter, delimitedNumbers);
}
I actually did this in two steps, one to pass each test. Our second scenario and passing implementation looks like this:
public class When_parsing_expression_without_delimiter_specifier :ConcernFor<DelimiterParser> {
private DelimitedNumbersExpression result;
private char[] defaultDelimiters;
private string expression;
[Test]
public void Should_return_default_delimiters() {
Assert.That(result.Delimiters, Is.EqualTo(defaultDelimiters));
}
[Test]
public void Should_return_numbers_expression_unchanged() {
Assert.That(result.DelimitedNumbers, Is.EqualTo(expression));
}
protected override void Because() {
result = sut.Parse(expression);
}
protected override void Context() {
defaultDelimiters = new[]{',', '\n'};
expression = "1,2,3,4";
}
protected override DelimiterParser CreateSubjectUnderTest() {
return new DelimiterParser();
}
}
//DelimiterParser.cs
public class DelimiterParser : IDelimiterParser {
private readonly char[] defaultDelimiters = new[] {',', '\n'};
private const string delimiterSpecifier = "//";
public DelimitedNumbersExpression Parse(string expression) {
if (!expression.StartsWith(delimiterSpecifier)) { return new DelimitedNumbersExpression(expression, defaultDelimiters);}
var delimiter = expression[2];
var delimitedNumbers = expression.Substring(4);
return new DelimitedNumbersExpression(delimitedNumbers, delimiter);
}
}
Our tests are now all green, and it looks like we have a good case for some refactoring. Let's use Extract Method to make our DelimiterParser a bit more readable:
public class DelimiterParser : IDelimiterParser {
private readonly char[] defaultDelimiters = new[] {',', '\n'};
private const string delimiterSpecifier = "//";
public DelimitedNumbersExpression Parse(string expression) {
if (!StartsWithDelimiterSpecifier(expression)) { return ExpressionWithDefaultDelimiters(expression);}
return ExpressionWithDelimitedNumbersAndCustomDelimiter(expression);
}
private DelimitedNumbersExpression ExpressionWithDelimitedNumbersAndCustomDelimiter(string expression) {
const int positionOfDelimiter = 2;
const int positionOfFirstNewLine = positionOfDelimiter + 1;
const int startOfDelimiterNumbers = positionOfFirstNewLine + 1;
var delimiter = expression[positionOfDelimiter];
var delimitedNumbers = expression.Substring(startOfDelimiterNumbers);
return new DelimitedNumbersExpression(delimitedNumbers, delimiter);
}
private DelimitedNumbersExpression ExpressionWithDefaultDelimiters(string expression) {
return new DelimitedNumbersExpression(expression, defaultDelimiters);
}
private bool StartsWithDelimiterSpecifier(string expression) {
return expression.StartsWith(delimiterSpecifier);
}
}
This is a longer class now, but I feel the Parse() method is much more understandable now. The ugliest bits are kept private.
A trivial factory implementation
Our CalculatorExpresionParser also needed an INumberParserFactory. As this will just be creating a NumberParser via a constructor, I don't see much benefit in unit testing it. You could write tests for it if you like, but I'm happy to assume that .NET constructors work ok. :)
public class NumberParserFactory : INumberParserFactory {
public INumberParser CreateWithDelimiters(char[] delimiters) {
return new NumberParser(delimiters);
}
}
This will require us to refactor our NumberParser to take constructor arguments. Let's update our spec for it:
//NumberParserSepcs.cs, in abstract Concern class:
protected override void Context() {
delimiters = new[] {',', '\n'};
}
protected override NumberParser CreateSubjectUnderTest() {
return new NumberParser(delimiters);
}
This also lets us simplify our NumberParser specifications too, as we no longer need to test specific delimiters (',' and '\n'), but just test that it splits on whatever delimiters it has been created with. Let's make this pass:
public class NumberParser : INumberParser {
public readonly char[] Delimiters;
public NumberParser(char[] delimiters) { Delimiters = delimiters; }
public IEnumerable<int> GetNumbers(string expression) {
if (expression == "") return new int[0];
return expression.Split(Delimiters).Select(x => int.Parse(x));
}
}
Please, not another spec! Have mercy!
Guess what? We're done. We don't have any unimplemented interfaces defined in specs. We should probably check this works now though. I'll add a default constructor to our Calculator class to wire up our dependencies (although for a real app we'd use an DI container).
//In Calculator.cs
public Calculator() {
IDelimiterParser delimiterParser = new DelimiterParser();
INumberParserFactory numberParserFactory = new NumberParserFactory();
_numberParser = new CalculatorExpressionParser(delimiterParser, numberParserFactory);
_adder = new Adder();
}
Just for good measure, I've added a file called IntegrationTests.cs and put in all the tests from our first attempt. And it all passed first go. Isn't that nice? :)
Post-mortem
So how'd this go? We ended up with a much more abstract design than our previous attempt. Because of this abstraction I felt code was easier to change, especially when it came to adding the custom delimiter behaviour. In that case we just picked the level of abstraction that seemed to work, then let the tests guide us. I also feel this level of abstractions helps prevent brittle tests. Because the majority of the test code focuses on the SUT's single responsibility, we only need to adjust the context to add or remove collaborators and change the behaviour.
I found that the BDD-style approach tends to force me to think more about design issues up front, and the tests give me guidance as to how to think through these issues. It makes it very easy to adhere to SOLID principles, and makes it clear when I am violating them.
I also found that when I got closer to the lowest levels of abstraction and I began hitting reality and the .NET framework code, I ended up reverting to a more TDD-style of testing. For example, the NumberParserSpecs test specific permutations of data, and it ends up just checking the use of the .NET String.Split() method and int.Parse(). I've had lots of trouble caused by focussing of testing data like this at the wrong levels of abstraction, so using the BDD-style was great for avoiding this until necessary. The majority of the time I could focus on telling dependencies to do something, not asking them for data and then acting on it.
On the negative side, we have what could reasonably be described as an over-engineered design . With 7 classes and 4 interfaces, perhaps "over-engineered" is an understatement. ;) That said, each class has a trivial implementation. With the possible exception of the DelimiterParser, you can pretty much just inspect each class for correctness. I didn't find that coding additional classes slowed me down at all. Because I got so much guidance from my tests the implementation was fairly quick. If it's just as fast and cheap to over-design, but you get the ability to more easily change your code, is that still over-design?
Of course, these posts have been more about looking at the how each approach influences the design direction, not if that level of design is a good idea. I'm quite sure you could easily end up with an identical design using either approach, but it is how each TDD style forces design decisions at different points that really interests me. I think the most illuminating part of this exercise from my perspective is that I need to work harder on my basic TDD skills -- at applying SOLID and being more aggressive in the refactoring step of the red-green-refactor cycle. Once I learn how to refactor both production and test code better, then I'll hopefully be able to move back and forward between styles as appropriate.
Hope you enjoyed this series. Would love to get your thoughts, so feel free to leave a comment, email me or tweet me @davetchepak.
