TDD - Kata - String Calculator

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


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

• Numbers bigger than 1000 should be ignored, so adding 2 + 1001 = 2

• Delimiters can be of any length with the following format: “//[delimiter]\n” for example: “//[]\n12***3” should return 6

• Allow multiple delimiters like this: “//[delim1][delim2]\n” for example “//[][%]\n12%3” should return 6.

• make sure you can also handle multiple delimiters with length longer than one char

Unit Tests:

```
[TestFixture]
public class StringCalculatorTests
{
private StringCalculator _underTest;

[SetUp]
pu

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...

StackExchange Code Review Q#128361, answer score: 3