String calculator kata

Question

I'm new to C# and just tried the String Calculator Kata for practice. What I like to know is if you (as more experienced C# programmers) have some suggestions for improvement with the end result (concerning readability, good practices, C# specific idioms etc.)

Code:

public class StringCalculator
{
    private const char DefaultDelimiter = ',';

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

        IEnumerable<string> tokens = Tokenize(input);
        IEnumerable<int> numbers = tokens.Select(ConvertToNumber).ToList();

        CheckForNegativeNumbers(numbers);

        return numbers.Sum();
    }

    private static void CheckForNegativeNumbers(IEnumerable<int> numbers)
    {
        List<int> negativeNumbers = numbers.Where(number => number < 0).ToList();

        if (negativeNumbers.Count > 0)
            throw new ArgumentException("Negatives not allowed: " + FormatNegativeNumbers(negativeNumbers));
    }

    private static string FormatNegativeNumbers(IEnumerable<int> negativeNumbers)
    {
        return string.Join(" ", negativeNumbers);
    }

    private static IEnumerable<string> Tokenize(string input)
    {
        char delimiter = DefaultDelimiter;

        if (CustomDelimiterSpecified(input))
        {
            delimiter = ParseCustomDelimiter(ref input);
        }
        else
        {
            input = ReplaceAlternativeDelimitersWithCommas(input);    
        }

        return input.Split(delimiter);
    }

    private static char ParseCustomDelimiter(ref string input)
    {
        char customDelimiter = input[2];
        input = input.Substring(4);
        return customDelimiter;
    }

    private static bool CustomDelimiterSpecified(string input)
    {
        return input.StartsWith("//");
    }

    private static string ReplaceAlternativeDelimitersWithCommas(string input)
    {
        return input.Replace("\n", ",");
    }

    private static int ConvertToNumber(string input)
    {
        return int.Parse(input);
    }
}

Tests:

[TestClass]
public class StringCalculatorTest
{
    private StringCalculator calculator = new StringCalculator();

    [TestMethod]
    public void Add_EmptyString_ReturnsZero()
    {
        int result = calculator.Add("");

        Assert.AreEqual(0, result);
    }

    [TestMethod]
    public void Add_OneNumber_ReturnsNumber()
    {
        int result = calculator.Add("42");

        Assert.AreEqual(42, result);
    }

    [TestMethod]
    public void Add_TwoNumbersDelimitedWithComma_ReturnsSum()
    {
        int result = calculator.Add("1,2");

        Assert.AreEqual(3, result);
    }

    [TestMethod]
    public void Add_MultipleNumbersDelimitedWithComma_ReturnsSum()
    {
        int result = calculator.Add("1,2,3");

        Assert.AreEqual(6, result);
    }

    [TestMethod]
    public void Add_TwoNumbersDelimitedWithNewLine_ReturnsSum()
    {
        int result = calculator.Add("1\n2");

        Assert.AreEqual(3, result);
    }


    [TestMethod]
    public void Add_TwoNumbersDelimitedWithCustomDelimiter_ReturnsSum()
    {
        string input = "//;\n1;2";
        int result = calculator.Add(input);

        Assert.AreEqual(3, result);
    }

    [TestMethod]
    [ExpectedException(typeof(ArgumentException))]
    public void Add_NegativeNumber_ThrowsArgumentException()
    {
        calculator.Add("-1");
    }

    [TestMethod]
    public void Add_NegativeNumber_ErrorMessageContainsNumber()
    {
        try
        {
            calculator.Add("-1");
            Assert.Fail();
        }
        catch (ArgumentException e)
        {
            Assert.AreEqual("Negatives not allowed: -1", e.Message);
        }
    }

    [TestMethod]
    public void Add_MultipleNegativeNumbers_ErrorMessageContainsAllNegativeNumbers()
    {
        try
        {
            calculator.Add("-1,2,-3");
            Assert.Fail();
        }
        catch (ArgumentException e)
        {
            Assert.AreEqual("Negatives not allowed: -1 -3", e.Message);
        }            
    }         
}

Answer 1

In regards to your implementation, I can see no reason that it needs to be an instance class so I would suggest making the class static. There is no reason to have to create a Calculator to call Add, it returns the result of the addition and maintains no state. If you were maintaining a running total and had a Subtract method etc then it would make sense as an instance class as you wouldn't want 2 separate callers to share the same running total.

This method may be better expressed as a Linq query:

private static void CheckForNegativeNumbers(IEnumerable<int> numbers)
{
    List<int> negativeNumbers = numbers.Where(number => number < 0).ToList();

    if (negativeNumbers.Count > 0)
        throw new ArgumentException("Negatives not allowed: " + FormatNegativeNumbers(negativeNumbers));
}

Using .Any() will return true as soon as it finds a match rather than having to enumerate the whole collection (I know in this case you will have a limited number of values in numbers but it's a good method to know about). Also, it saves having to create a list just to check the count so it's more efficient too (the amount by which will depend on the implementation and number of values, but again it's a good practice to stick to where it makes sense).

private static void CheckForNegativeNumbers(IEnumerable<int> numbers)
{
    if (numbers.Any(number => number < 0))
    {
        throw new ArgumentException("Negatives not allowed: " + FormatNegativeNumbers(negativeNumbers));
    }
}

Also, avoid inline or unbraced if conditions - always wrap the body in braces. It makes it easier to follow the code and helps prevent bugs being introduced at a later stage if additional actions needs to happen inside the same if body.

In terms of your tests, I have a few suggestions.

Firstly, change these two tests:

[TestMethod]
[ExpectedException(typeof(ArgumentException))]
public void Add_NegativeNumber_ThrowsArgumentException()
{
    calculator.Add("-1");
}

[TestMethod]
public void Add_NegativeNumber_ErrorMessageContainsNumber()
{
    try
    {
        calculator.Add("-1");
        Assert.Fail();
    }
    catch (ArgumentException e)
    {
        Assert.AreEqual("Negatives not allowed: -1", e.Message);
    }
}

The ExpectedException attribute is known to be problematic as you can't verify which method call has thrown an exception in the case that you have multiple method calls (see this blog post for further details). Another problem is the one you are facing in the second method whereby you want to verify the exception. If your testing framework has it (both NUnit and xUnit do) use Assert.Throws instead.

var exception = Assert.Throws<ArgumentException>(() => calculator.Add("-1"));

This allows you to verify the exact method that should fail and also capture the exception if you want to check the message, param name etc.

Secondly, stop using a shared instance of calculator across all tests. They should all be isolated from each other. One test method should not have the ability to potentially alter the state for another test method. I know in this example, there is no shared state in calculator and in this instance it would be alleviated by making the class static as I mentioned above, but it's a good practice to get into regardless.

Personally I consider calling Assert.Fail() a code smell, it's use should be carefully considered to see if there is a different way the test can be written. If for whatever reason you decide that you need to call it, at least use the overload to specify a failure reason so that if the test fails you have some clue as to why: Assert.Fail("An ArgumentException should have been thrown since we have called Add with a negative number");.

Something else you may want to consider as far as test names go is re-wording the test names to be more contextual to the desired behaviour. For example Add_EmptyString_ReturnsZero could be expressed as WhenCallingAdd_WithAnEmptyString_ZeroShouldBeReturned the method name is slightly longer but it defines the specification of the behaviour. This will help you (and other developers) understand how your application should behave at a later stage.

Answer 2

I think your answer was pretty well done. Really, for that test, you probably got near full marks. I know that wasn't much of an answer, but there really wasn't much to add to your code.

Answer 3

I like your solution as well. The only thing I might consider doing differently is making input a private instance variable which would mean all your input parameter requirements to the methods would no longer be required.

Something like:

public class StringCalculator
{
    private const char DefaultDelimiter = ',';
    private string _input = "";

    public int Add(string input)
    {
        _input = input;

        if (string.IsNullOrWhiteSpace(_input))
            return 0;

        IEnumerable<string> tokens = Tokenize();
        IEnumerable<int> numbers = tokens.Select(ConvertToNumber).ToList();

        CheckForNegativeNumbers(numbers);

        return numbers.Sum();
    }

    private static void CheckForNegativeNumbers(IEnumerable<int> numbers)
    {
        var negativeNumbers = numbers.Where(number => number < 0).ToList();

        if (negativeNumbers.Any())
            throw new ArgumentException("Negatives not allowed: " + FormatNegativeNumbers(negativeNumbers));
    }

    private static string FormatNegativeNumbers(IEnumerable<int> negativeNumbers)
    {
        return string.Join(" ", negativeNumbers);
    }

    private IEnumerable<string> Tokenize()
    {
        char delimiter = DefaultDelimiter;

        if (CustomDelimiterSpecified())
        {
            delimiter = ParseCustomDelimiter();
        }
        else
        {
            ReplaceAlternativeDelimitersWithCommas();
        }

        return _input.Split(delimiter);
    }

    private char ParseCustomDelimiter()
    {
        char customDelimiter = _input[2];
        _input = _input.Substring(4);
        return customDelimiter;
    }

    private bool CustomDelimiterSpecified()
    {
        return _input.StartsWith("//");
    }

    private void ReplaceAlternativeDelimitersWithCommas()
    {
        _input = _input.Replace("\n", ",");
    }

    private static int ConvertToNumber(string input)
    {
        int intValue;
        return int.TryParse(input, out intValue) ? intValue : 0;            
    }
}