String Calculator Kata Code in TDD style

Question

I have done the following kata in TDD style and would appreciate it if someone could review my code and my tests.

String Calculator

• Create a simple String calculator with a method int Add(string numbers). The method can take 0, 1 or 2 numbers, and will return their sum (for an empty string it will return 0). For example "" or "1" or "1,2".

• Start with the simplest test case of an empty string and move to 1 and two numbers

• Remember to solve things as simply as possible so that you force yourself to write tests you did not think about.

• Remember to refactor after each passing test.

• Allow the Add method to handle an unknown amount of numbers.

  Allow the Add method to handle new lines between numbers (instead of commas). The following input is ok: "1\n2,3" (will equal 6). The following input is NOT ok: "1,\n" (not need to prove it - just clarifying).

• Support different delimiters to change a delimiter, the beginning of the string will contain a separate line that looks like this: "//[delimiter]\n[numbers…]". For example, "//;\n1;2" should return three where the default delimiter is ';'. The first line is optional. All existing scenarios should still be supported.

• Calling Add with a negative number will throw an exception "negatives not allowed" - and the negative that was passed. If there are multiple negatives, show all of them in the exception message.

public class StringCalculator
    {
        public int AddNumbers(string args)
        {
            if (string.IsNullOrEmpty(args))
            {
                return 0;
            }

            var delimeters = new List<char>()
                                    {
                                        '\n',','
                                    };

            if (args[0] == '/')
            {
                var customDelimeter = args[2];
                delimeters.Add(customDelimeter);
                args = args.Remove(0,
                                   3);

            }

            var numbers = args.ToCharArray().Where(x => !delimeters.Contains(x)).ToList();

            if (numbers.Any(x => x == '-'))
            {
                StringBuilder stringBuilder= new StringBuilder();
                for (int i = 0;
                     i < numbers.Count;
                     i++)
                {
                    if (numbers[i] == '-')
                    {
                        stringBuilder.Append("-");
                        stringBuilder.Append(numbers[++i]);
                        stringBuilder.Append(", ");
                    }    
                }

                throw new Exception(string.Format("negatives {0} not allowed",stringBuilder.ToString()));
            }

            var sum = numbers.Sum(x => (int)Char.GetNumericValue(x));

            return sum;
        }
    }

Tests:

 [TestFixture]
    public class StringCalculatorTests
    {
        [Test]
        public void ShouldReturnZeroForEmptyString()
        {
            var sut = new StringCalculator();
            var result = sut.AddNumbers("");
            Assert.AreEqual(0,result);
        }

        [Test]
        [TestCase(1,"1")]
        [TestCase(2,"2")]
        public void ShouldReturnNumberIfGivenOneNumber(int expected,string arg)
        {
            var sut = new StringCalculator();
            var result = sut.AddNumbers(arg);
            Assert.AreEqual(expected,result);
        }

        [Test]
        [TestCase(3, "1,2")]
        [TestCase(11, "1,2,3,5")]
        public void ShouldReturnSumOfAllNumbers(int expected, string arg)
        {
            var sut = new StringCalculator();
            var result = sut.AddNumbers(arg);
            Assert.AreEqual(expected, result);
        }

        [Test]
        [TestCase(3, "1\n2")]
        [TestCase(11, "1,2\n3,5")]
        public void ShouldAllowNewLineAsASeparator(int expected, string arg)
        {
            var sut = new StringCalculator();
            var result = sut.AddNumbers(arg);
            Assert.AreEqual(expected, result);
        }

        [Test]
        [TestCase(3, "//;\n1;2")]
        [TestCase(11, "//.\n1.2\n3.5")]
        [TestCase(11, "//-\n1-2\n3-5")]
        public void ShouldSupportDifferentSeparators(int expected, string arg)
        {
            var sut = new StringCalculator();
            var result = sut.AddNumbers(arg);
            Assert.AreEqual(expected, result);
        }

        [Test]
        [TestCase(3, "//;\n-1;2")]
        [TestCase(3, "//;\n-1;-2")]
        [ExpectedException(typeof(Exception))]
        public void ShouldThrowExceptionIfNegativeInArgs(int expected, string arg)
        {
            var sut = new StringCalculator();
            var result = sut.AddNumbers(arg);
            Assert.AreEqual(expected, result);
        }
    }

Answer 1

Single responsibility principle

The method AddNumbers() violates the SRP because it is

• parsing arguments
• composing exception messages
• adding the numbers

this should be done in separate methods like

private bool ContainsCustomDelimiter(string argument)
private string GetCustomDelemiter(string argument)
private string ComposeExceptionMessage(IEnumerable<char> numbers)  

by splitting this method into smaller methods which have a defined responsibility your code will be easier to maintain and extend.

---

Number or digit ?

Thats the question which bothers me the most. I think of numbers like 1, 3, 12, 56 but it looks like the method will only sum separated digits which should be clearly stated inside a documentation.

---

Tests

The testname ShouldReturnZeroForEmptyString() is misleading or the AddNumbers() method does not return the expected result.

The method can take 0, 1 or 2 numbers, and will return their sum (for an empty string it will return 0) for example “” or “1” or “1,2”

But an empty string is not a null string, nevertheless the AddNumbers() method is returning 0 for a null value also because of

if(string.IsNullOrEmpty(args))  

In my opinion the AddNumbers() method should throw an NullReferenceException for the case that the passed in string is null.

So you better check for

if (args.Length == 0) { return 0; } 

which will throw the NRE if args is null

But there is another big problem. Assume you pass one of the following strings to the AddNumbers() method:

• "/" -> throws IndexOutOfRange
• "//" -> throws IndexOutOfRange

You need to always check for such edge cases, thats what tests are for.

---

Now let us talk about cases where the input is not valid.

What should happen for a given string like "1, 2,3" or "1,2,A" ? The first will return 5 and the second will return 5 too.

So you should better check numbers if they contain any non digit and handle this case.

---

Naming

Naming is important because it tells you (if done correctly) at first glance what a variable is about. Bob the maintainer will have a hard time if he/she sees a variable named sut (but only if he/she doesn't know (like me) that sut stands for "system under test"). But nevertheless why don't you name it calculator ?

Answer 2

Some things that come to mind are:

• Substitute var numbers = args.ToCharArray().Where(x => !delimeters.Contains(x)).ToList(); with var numbers = args.Where(x => !delimeters.Contains(x)); unless you specifically want a list. The ToCharArray() call should be superfluous.
• Substitute if (numbers.Any(x => x == '-')) with if (numbers.Contains('-')) for readability's sake.
• The command stringBuilder.Append(numbers[++i]); makes it so that i gets incremented twice in each cycle. Is it intentional? If not, you should remove the extra ++ and substitute it with i + 1.

Other than these, I don't see any problems in the code.

Answer 3

Apart from what's already been said you should consider your test suite a bit more (I'm assuming you're using NUnit in pretty recent version).

Apply DRY to the tests

You have two more or less identical lines in all tests:

var sut = new StringCalculator();
var result = sut.AddNumbers(...);

Declare the calculator instance as a field in the fixture instead, and initialize in a [SetUp] method.

There's also the duplication of var result = .... In this specific case I might go as far as to get rid of result entirely:

public void ShouldXxx(int expected,string arg)
{
    Assert.AreEqual(expected, calculator.AddNumbers(arg));
}

While I'm at it this reads even better, IMHO:

public void ShouldXxx(int expected, string arg)
{
    Assert.That(calculator.AddNumbers(arg), Is.EqualTo(expected));
}

Try reading the code out loud for yourself for each of the samples above.

Further adding a few line breaks to the latest version makes it a bit easier to visually separate what you're testing from the expected result, but that's a very personal opinion. See refactored example below.

Don't use SUT literally

As others have already said, don't use sut as a variable name. Sure, you might know the context and be familiar with the acronym but I still consider calculator.AddNumbers(...) to be more expressive than sut.AddNumbers().

One circumstance where you could use e.g. sut as a variable name is where you have a very generic test suite that can be reused for many implementations. However, even then I would strongly suggest that you name the variable referring to the SUT according to what kind of capabilities being tested within that specific suite. A short example:

public abstract class CloneableTests<T> where T: ICloneable
{
    private T cloneable; // Not 'sut'
    ....
}

Remove unneeded attributes

You don't need to use the [TestFixture] attribute. Marking individual methods with [SetUp] or one of the [Test...] attributes is sufficient. Also, using [Test] when [TestCase] is present is redundant.

Here's an excerpt from the refactored, and somewhat reformatted, tests:

public class StringCalculatorTests
{
    private StringCalculator calculator;

    [SetUp]
    public void InitFixture()
    {
        calculator = new StringCalculator();
    }

    [Test]
    public void ShouldReturnZeroForEmptyString()
    {
        Assert.That(
            calculator.AddNumbers(String.Empty),
            Is.EqualTo(0)
        );
    }

    [TestCase(1, "1")]
    [TestCase(2, "2")]
    public void ShouldReturnNumberIfGivenOneNumber(int expected, string arg)
    {
        Assert.That(
            calculator.AddNumbers(arg),
            Is.EqualTo(expected)
        );
    }

    ...
}