TDD - String Calculator Kata

Question

Would really appreciate it if someone could please review my String Calculator TDD KATA.

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.

Tests:

public class StringCalculatorTests
    {
        Calculator.StringCalculator stringCalculator;

        [SetUp]
        public void SetUp()
        {
            stringCalculator = new Calculator.StringCalculator();
        }

        [Test]
        [ExpectedException(typeof (ArgumentNullException))]
        public void TestAddWithNullStringThrowsException()
        {
            stringCalculator.Add(null);
        }

        [Test]
        public void TestAddWithEmptyStringReturnsZero()
        {
            string numbers = string.Empty;
            int result = stringCalculator.Add(numbers);
            Assert.AreEqual(0,
                            result);
        }

        [Test]
        public void TestAddWithSingleNumberInStringReturnsTheNumber()
        {
            string numbers = "1";
            int result = stringCalculator.Add(numbers);
            Assert.AreEqual(1,
                            result);
        }

        [TestCase("1,2,3,4,5,6,7,8,9,10")]
        public void TestAddWithMultipleNumbersInStringReturnsTheTotal(string inputNumbers)
        {
            int result = stringCalculator.Add(inputNumbers);
            Assert.AreEqual(55,
                            result);
        }

        [TestCase("1,2,3,4,5\n6,7,8\n9,10")]
        public void TestAddWithMultipleNumbersInStringSplitWithNewLinesAndCommasReturnsTheTotal(string inputNumbers)
        {
            int result = stringCalculator.Add(inputNumbers);
            Assert.AreEqual(55,
                            result);
        }

        [TestCase("1,2,3,4,\n6,7,\n9,10")]
        public void TestAddWithMultipleNumbersInStringReturnZeroForEmptyNumbers(string inputNumbers)
        {
            int result = stringCalculator.Add(inputNumbers);
            Assert.AreEqual(42,
                            result);
        }

        [TestCase("//;\n1;2")]
        public void TestAddWithMultipleNumbersInStringUsingDifferentDelimeter(string inputNumbers)
        {
            int result = stringCalculator.Add(inputNumbers);
            Assert.AreEqual(3,
                            result);
        }

        [TestCase("//;\n1;-2")]
        [ExpectedException(typeof (ArgumentException))]
        public void TestAddWithNegativeNumbersWillThrowException(string inputNumbers)
        {
            int result = stringCalculator.Add(inputNumbers);
            Assert.AreEqual(3,
                            result);
        }
    }

Implementation:

public class StringCalculator
    {
        public int Add(string numbers)
        {
            ValidateInputNumbers(numbers);

            var integers = ConvertStringToIntegers(numbers);

            var total = integers.Sum(x => x);

            return total;
        }

        private static void ValidateInputNumbers(string numbers)
        {
            if (numbers == null)
            {
                throw new ArgumentNullException("numbers");
            }
        }

        private static IEnumerable<int> ConvertStringToIntegers(string numbers)
        {
            var integers = new List<int>();

            if (numbers.Trim()
                       .Length == 0)
            {
                integers.Add(0);
            }
            else
            {
                var delimeters = FormatInputNumbers(ref numbers);


                integers = numbers.Split(delimeters.ToArray())
                                  .Where(x => x.Length > 0)
                                  .Select(x => Convert.ToInt32(x))
                                  .ToList();

                var negativeNumbers = integers.Where(x => x < 0)
                                              .ToList();
                if (negativeNumbers.Any())
                {
                    throw new ArgumentException(string.Format("negatives {0} not allowed",
                                                              string.Join(",",
                                                                          negativeNumbers)));
                }
            }

            return integers;
        }

        private static List<char> FormatInputNumbers(ref string numbers)
        {
            var delimeters = new List<char>
                             {
                                 ',',
                                 '\n'
                             };

            if (numbers[0] == '/' && numbers[1] == '/' && numbers[3] == '\n')
            {
                delimeters.Add(numbers[2]);
                numbers = numbers.Remove(0,
                                         3);
            }
            return delimeters;
        }
    }

Answer 1

Aesthetics: Vertical WhiteSpace

I don't understand the need for a new line here:

Assert.AreEqual(0,
                result);

Or here:

if (numbers.Trim()
           .Length == 0)

Or here:

  string.Join(",",
              negativeNumbers)));

Or there:

numbers = numbers.Remove(0,
                         3);

It's hurting readability IMO, but that's just my opinion.

---

Redundant, overkill or otherwise unneeded stuff

That said, [String].Trim().Lengh == 0 is overly verbose, compared to [String].IsNullOrWhiteSpace().

This variable isn't needed, and the vertical whitespace is abused:

        var total = integers.Sum(x => x);

        return total;

The variable and the statement that follows are very closely related, and should read like this:

        var total = integers.Sum(x => x);
        return total;

But then again, total isn't needed, nor is the selector lambda. Why not just do this?

return integers.Sum();

FormatInputNumbers has no reason to return a List<T> - its caller only cares that it's IEnumerable<T>.

---

This bit of logic needs an explanatory comment:

    if (numbers[0] == '/' && numbers[1] == '/' && numbers[3] == '\n')
    {
        delimeters.Add(numbers[2]);
        numbers = numbers.Remove(0,
                                 3);
    }

---

Static

I don't like static code, which I find especially contradicting with TDD. Where you see implementation details of a StringCalculator object, I see a dependency on a specialized object that performs the conversion - with its own public interface and corresponding tests.

The validation one-liner is really nothing more than a guard clause, and should be inlined IMO.

Answer 2

1. FormatInputNumbers is poorly named, it actually parses delimiters.
2. To test for the empty string case, you could instead use string.IsNullOrWhiteSpace(string s) which tests for all of the different types of whitespace chars.
3. Rather than having a large if/else statement, you could simply return a new array, return new[] { 0 };
4. I think it would be more straight forward if you split the input text on the newline character, this would give you var lines = string[]. Then you could test the first line for the leading '//' and update the delimiters as necessary. Then for the rest of the lines (including the first if it wasn't a delimiter declaration) could be split on the delimiter, evaluated, and added to the list if it's a number.