String Calculator Kata Code in TDD style

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()
{
'\n',','
};

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

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

StackExchange Code Review Q#93076, answer score: 8