TDD - Kata - String Calculator

Question

This is my first TDD Kata I've done and I'd appreciate it if someone could review it.

String Calculator

1. Create a simple String calculator with a method int Add(string numbers)
   1. 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”
   2. Start with the simplest test case of an empty string and move to 1 and two numbers
   3. Remember to solve things as simply as possible so that you force yourself to write tests you did not think about
   4. Remember to refactor after each passing test
2. Allow the Add method to handle an unknown amount of numbers
3. Allow the Add method to handle new lines between numbers (instead of commas).
   1. the following input is ok: “1\n2,3” (will equal 6)
   2. the following input is NOT ok: “1,\n” (not need to prove it - just clarifying)
4. Support different delimiters
   1. 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 ‘;’.
   2. the first line is optional. all existing scenarios should still be supported
5. 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
6. Numbers bigger than 1000 should be ignored, so adding 2 + 1001 = 2
7. Delimiters can be of any length with the following format: “//[delimiter]\n” for example: “//[***]\n1***2***3” should return 6
8. Allow multiple delimiters like this: “//[delim1][delim2]\n” for example “//[*][%]\n1*2%3” should return 6.
9. make sure you can also handle multiple delimiters with length longer than one char

Unit Tests:

[TestFixture]
public class StringCalculatorTests
{
    private StringCalculator _underTest;

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

    private int Act_CalculateNumbers(string numbers)
    {
        var calculatedResult = _underTest.Add(numbers);

        return calculatedResult;
    }

    [Test]
    [TestCase("", ExpectedResult = 0)]
    [TestCase(null, ExpectedResult = 0)]
    public int Returns_0_When_Null_Or_EmptyString_Input(string numbers)
    {
        return Act_CalculateNumbers(numbers);
    }

    [Test]
    [TestCase("1", ExpectedResult = 1)]
    [TestCase("100", ExpectedResult = 100)]
    public int Returns_SameNumber_When_ValidNumber_Input(string numbers)
    {
        return Act_CalculateNumbers(numbers);
    }

    [Test]
    [TestCase("1,2", ExpectedResult = 3)]
    [TestCase("11,12", ExpectedResult = 23)]
    public int Returns_SumOf_TwoNumbers_When_Two_ValidNumbers_Input(string numbers)
    {
        return Act_CalculateNumbers(numbers);
    }

    [Test]
    [TestCase("1,2,3,4", ExpectedResult = 10)]
    [TestCase("11,12,13,14", ExpectedResult = 50)]
    public int Returns_SumOf_Numbers_When_Multiple_ValidNumbers_Input(string numbers)
    {
        return Act_CalculateNumbers(numbers);
    }

    [Test]
    [TestCase("1\n2", ExpectedResult = 3)]
    [TestCase("1\n14", ExpectedResult = 15)]
    [TestCase("1\n1,4", ExpectedResult = 6)]
    [TestCase("1\n5,4", ExpectedResult = 10)]
    public int Returns_CorectSum_When_NewLine_Delimiter_Used(string numbers)
    {
        return Act_CalculateNumbers(numbers);
    }

    [Test]
    [TestCase("//$\n1", ExpectedResult = 1)]
    [TestCase("//$\n1$2", ExpectedResult = 3)]
    [TestCase("//$\n1$2,3", ExpectedResult = 6)]
    [TestCase("//$\n1$2,3\n4", ExpectedResult = 10)]
    [TestCase("//$\n1$2,3\n4$5", ExpectedResult = 15)]
    public int Returns_CorectSum_When_Custom_Delimiter_Used(string numbers)
    {
        return Act_CalculateNumbers(numbers);
    }

    [Test]
    [TestCase("-1", "-1")]
    [TestCase("1,-1", "-1")]
    [TestCase("1\n-1", "-1")]
    [TestCase("//$\n-1", "-1")]
    [TestCase("//$\n1$-2", "-2")]
    [TestCase("//$\n1$-2,3", "-2")]
    [TestCase("//$\n1$-2,-3\n4", "-2,-3")]
    [TestCase("//$\n1$2,3\n4$-5", "-5")]
    public void Throws_Correct_Exception_When_NegativeNumber_Input(string numbers, string negativeNumbers)
    {
        //Act
        var exception = Should.Throw<FormatException>(() => Act_CalculateNumbers(numbers));

        //Assert
        exception.Message.ShouldBe($"negatives not allowed '{negativeNumbers}'");
    }

    [Test]
    [TestCase("1001", ExpectedResult = 0)]
    [TestCase("1,1001", ExpectedResult = 1)]
    [TestCase("1\n1001", ExpectedResult = 1)]
    [TestCase("//$\n1,1001", ExpectedResult = 1)]
    [TestCase("//$\n1$1001", ExpectedResult = 1)]
    [TestCase("//$\n1$2,1001", ExpectedResult = 3)]
    public int Returns_CorrectSum_When_Ignoring_Numbers_Greater_Than_1000(string numbers)
    {
        return Act_CalculateNumbers(numbers);
    }

    [Test]
    [TestCase("//[$$][££]\n1££1", ExpectedResult = 2)]
		[TestCase("//[$$][££]\n1$$1££1", ExpectedResult = 3)]
		[TestCase("//[$$][££]\n1$$1,1££1", ExpectedResult = 4)]
		[TestCase("//[$$$][£££]\n1$$$1,1\n1£££1", ExpectedResult = 5)]
    public int Returns_CorrectSum_With_Custom_Delimiters_Of_Any_Length(string numbers)
    {
        return Act_CalculateNumbers(numbers);
    }
}

Implementation:

public class StringCalculator
{
    private readonly List<string> _defaultDelimiters = new List<string> { ",", "\n" };
    private const int StartIndexOfNumbersWithCustomDelimiter = 3;
    private const int StartIndexOfCustomDelimiter = 2;
    private const int MaxNumberLimit = 1000;
    private const string CustomDelimiterIdentifier = "//";

    public int Add(string numbers)
    {
        if (string.IsNullOrEmpty(numbers)) return 0;

        if (numbers.StartsWith(CustomDelimiterIdentifier))
        {
            numbers = GetNumbersExcludingCustomDelimiter(numbers);
        }

        var sumOfNumbers = GetSumOfNumbers(numbers);

        return sumOfNumbers;
    }

    private int GetSumOfNumbers(string numbers)
    {
        var convertedNumbers = numbers.Split(_defaultDelimiters.ToArray(), StringSplitOptions.None).Select(int.Parse).ToList();

        ValidateNumbersArePositive(convertedNumbers);

        var sumOfNumbers = convertedNumbers.Where(x => x <= MaxNumberLimit).Sum();
        return sumOfNumbers;
    }

    private string GetNumbersExcludingCustomDelimiter(string numbers)
    {
        var startIndexOfString = AssignCustomDelimiterAndReturnStartIndexOfNumbers(numbers);

        numbers = numbers.Substring(startIndexOfString);
        return numbers;
    }

    private int AssignCustomDelimiterAndReturnStartIndexOfNumbers(string numbers)
    {
        var customDelimiters = GetCustomDelimiter(numbers);
        _defaultDelimiters.AddRange(customDelimiters);

        var hasMultipleDelimiters = customDelimiters.Count > 1;
        var multipleDelimiterLength = hasMultipleDelimiters ? (customDelimiters.Count * 2) : 0;

        return StartIndexOfNumbersWithCustomDelimiter + customDelimiters.Sum(x => x.Length) + multipleDelimiterLength;
    }

    private static void ValidateNumbersArePositive(IReadOnlyCollection<int> convertedNumbers)
    {
        if (!convertedNumbers.Any(x => x < 0)) return;

        var negativeNumbers = string.Join(",", convertedNumbers.Where(x => x < 0).Select(x => x.ToString()).ToArray());
        throw new FormatException($"negatives not allowed '{negativeNumbers}'");
    }

    private static IList<string> GetCustomDelimiter(string numbers)
    {
        var allDelimiters = numbers.Substring(StartIndexOfCustomDelimiter, numbers.IndexOf('\n') - StartIndexOfCustomDelimiter);

        var splitDelimiters = allDelimiters.Split('[').Select(x => x.TrimEnd(']')).ToList();

        if (splitDelimiters.Contains(string.Empty))
        {
            splitDelimiters.Remove(string.Empty);
        }

        return splitDelimiters;
    }
}

Answer 1

One of my goals when I'm using TDD is to try to clarify the requirements of the system I'm developing. The requirements you're working from have several unclear elements. You've addressed at least one of them (your requirements don't indicate what to do when a null string is supplied, but you have decided to treat it like an empty string, so you have a test for it). This is good, but because you are dealing with strings there are other cases that you haven't dealt with.

For example, what happens if the delimiter supplied uses one of the delimiter characters:

[TestCase("//[[][]]\n1[2]3", ExpectedResult = 6)]

What happens if a '-' is used as the delimeter?

[TestCase("//[-]\n1-2-3", ExpectedResult = 6)]
[TestCase("//[-]\n1--2-3", ExpectedResult = 4)]

What happens if a new line character is used as the delimiter?

[TestCase("//\n\n1\n3", ExpectedResult = 4)]

What happens if an invalid delimiter is passed in the numbers?

[TestCase("//.\n1.3|4", ExpectedResult = ?FormatException?)]

What happens if a leading or trailing delimiter is supplied?

[TestCase("//[*][%]\n1*2%3*", ExpectedResult = 6)]
[TestCase("//[*][%]\n*1*2%3", ExpectedResult = 6)]

My reading of the specification suggested that by supplying a delimiter list it should replace the existing default list. Your interpretation seems to be that it is additive, so the default delimiters are used as well as those supplied. I think this is ok, however I'd prefer to see a test that explicitly indicates that this is required behaviour, rather than just having it in a test case along with the other delimiter tests:

// Test Customer Delimiter Used
[TestCase("//$\n1", ExpectedResult = 1)]
[TestCase("//$\n1$2", ExpectedResult = 3)]
public int Returns_CorrectSum_When_Custom_Delimiter_Used(string numbers)

// Test Custom Delimiter doesn't override default delimiters
[TestCase("//$\n1$2,3", ExpectedResult = 6)]
[TestCase("//$\n1$2,3\n4", ExpectedResult = 10)]
[TestCase("//$\n1$2,3\n4$5", ExpectedResult = 15)]
public int Returns_CorrectSum_When_Custom_And_Default_Delimiter_Used(string numbers)

One of the test cases specified in your specification is:

[TestCase("//[*][%]\n1*2%3", ExpectedResult=6)]

You have your own version of this, however I would tend to use the exact case where possible, which you haven't done.

Your tests for ignoring numbers > 1000 only use one value (1001), only have a single instance of the value in the string and only have the value at the end of the string. This could be simply implemented as ignore 1001. Think about naive implementations when writing your cases in order to make sure the implementation does what's expected:

[TestCase("//$\n1$2,1001$7$10022$10", ExpectedResult = 20)]

As far as your actual code goes, there are a couple of things that stand out (but are mostly stylistic).

In a few places you use unnecessary local variables:

var sumOfNumbers = GetSumOfNumbers(numbers);

return sumOfNumbers;

Could just as well be:

return GetSumOfNumbers(numbers);

Unless you're stepping through the code, the local variables just add to the noise.

You have a method GetNumbersExcludingCustomDelimiter. In my head, Get methods don't change member variables. The first thing the method does is call AssignCustomDelimiterAndReturnStartIndexOfNumbers which updates the _defaultDelimiters. This feels wrong to me, but could just be a style thing...