TDD - String Calculator Kata

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))]
{
}

[Test]
{
string numbers = string.Empty;
Assert.AreEqual(0,
result);
}

[Test]
{
string numbers = "1";
Assert.AreEqual(1,
result);
}

[TestCase("1,2,3,4,5,6,7,8,9,10")]
{
Assert.AreEqual(55,
result);
}

[TestCase("1,2,3,4,5\n6,7,8\n9,10")]
{
Assert.AreEqual(55,
result);
}

[TestCase("1,2,3,4,\n6,7,\n9,10")]
{
Assert.AreEqual(42,
result);
}

[TestCase("//;\n1;2")]
{
Assert.AreEqual(3,
result);
}

[TestCase("//;\n1;-2")]
[ExpectedException(typeof (ArgumentException))]
{
Assert.AreEqual(3,
result);
}
}


Implementation:

public class StringCalculator
{
{
ValidateInputNumbers(numbers);

var integers = ConvertStringToIntegers(numbers);

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

}

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)
{
}
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')
{
numbers = numbers.Remove(0,
3);
}
return delimeters;
}
}

• The same but in Python codereview.stackexchange.com/questions/93842/… Jun 17, 2015 at 9:52
• You should link to your previous attempt. Also, IMHO it would be nice if you would just use the same basic title and add e.g. " #2" in order to establish some kind of temporal relation between the questions. Jun 18, 2015 at 8:06

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);



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

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


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')
{
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.

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.