8
\$\begingroup\$

I implemented Roy Osherove's TDD Kata 1: String Calculator using Python 3.5.2 and the unittest module. The full description can be found at the link, it's a bit lengthy for this post.

The jest of it is to implement various string parsing and simple numbers logic to add numbers together which are passed as a delimited string, e.g.: "1,2,3" returns 6.


I followed true TDD principles to the best of my knowledge, in that for each requirement I first wrote a failing test, then made it pass, rinse & repeat, refactoring along the way.

Note that test_incorrect_input_types was not part of the requirement, but rather is a test that I do out of habit to make sure the correct type is passed to the function.

I'm interested in any and all ways I could improve this code. In particular, I'm looking for ways to make the TDD approach as clear and solid as possible.


Unit tests:

import unittest

class TestAddStringNumbers(unittest.TestCase):
    """Tests for `add_string_numbers` function."""

    def test_incorrect_input_types(self) -> None:
        """Test that non-string inputs fail"""
        incorrect_inputs = [
            None,
            42,
            3.1415,
            [],
            {}
        ]
        for input_ in incorrect_inputs:
            self.assertRaises(TypeError, add, input_)

    def test_empty_string(self) -> None:
        """Test empty string as input"""
        input_ = ""
        expected = 0
        actual = add(input_)
        self.assertEqual(expected, actual)

    def test_1_number(self) -> None:
        """Test a single number as input"""
        input_ = "42"
        expected = 42
        actual = add(input_)
        self.assertEqual(expected, actual)

    def test_2_numbers(self) -> None:
        """Test 2 numbers as input"""
        input_ = "42,99"
        expected = 141
        actual = add(input_)
        self.assertEqual(expected, actual)

    def test_many_numbers(self) -> None:
        """Test many numbers as input"""
        input_ = "1,2,3,4,5,6,7,8,9,10"
        expected = 55
        actual = add(input_)
        self.assertEqual(expected, actual)

    def test_negative_numbers(self) -> None:
        """Test that input with negative numbers fails"""
        input_ = "-1"
        self.assertRaises(ValueError, add, input_)
        input_ = "-1,2,-3"
        self.assertRaises(ValueError, add, input_)

    def test_numbers_greater_than_1000(self) -> None:
        """Test that numbers greater than 1000 are ignored"""
        input_ = "1,1001,2"
        expected = 3
        actual = add(input_)
        self.assertEqual(expected, actual)
        input_ = "1001,2001,3001"
        expected = 0
        actual = add(input_)
        self.assertEqual(expected, actual)

    def test_newline_delimiter(self) -> None:
        """Test input separated by newlines instead of commas"""
        input_ = "1\n2\n3"
        expected = 6
        actual = add(input_)
        self.assertEqual(expected, actual)
        input_ = """1
        2
        3"""
        expected = 6
        actual = add(input_)
        self.assertEqual(expected, actual)

    def test_invalid_delimiters(self) -> None:
        """Test that input with invalid delimiter placement fails"""
        input_ = "1,,2"
        self.assertRaises(ValueError, add, input_)
        input_ = "1,2,"
        self.assertRaises(ValueError, add, input_)
        input_ = ",1,2"
        self.assertRaises(ValueError, add, input_)

    def test_custom_delimiters(self) -> None:
        """Test that custom delimiters can be used with `//[delim1][delim2]\n` syntax"""
        input_ = "//[***]\n1***2***3"
        expected = 6
        actual = add(input_)
        self.assertEqual(expected, actual)
        input_ = "//[***][&&]\n1***2&&3"
        expected = 6
        actual = add(input_)
        self.assertEqual(expected, actual)

Implementation:

from typing import List
import re

def add_string_numbers(numbers: str) -> int:
    """
    Add numbers passed in the form of a delimited string of numbers.
    For example, "1,2,3" returns 6.

    Delimiters are handled in the following ways:
    - The standard delimiter is a comma `,`
    - Newlines `\n` are always acceptable delimiters, e.g.:
        1\n2\n3
    - Additional delimiters can be declared with the following syntax
      added before the beginning of a delimited string:
        //[delim1][delim2]\n
      ...then used as delimiters. For example:
        //[***][&&]\n1***2&&3
      which will be parsed to:
        1,2,3
    """
    if not isinstance(numbers, str):
        raise TypeError("Input must be of string type")
    if numbers == "":
        return 0
    number_list = parse_into_list(numbers)
    result = 0
    negatives = []
    for num in number_list:
        try:
            num = int(num)
        except ValueError:
            raise ValueError("Input could not be converted to int")
        if num < 0:
            negatives.append(num)
        else:
            if num <= 1000:
                result += num
    if negatives:
        raise ValueError("Negatives not allowed: {0}".format(",".join(map(str, negatives))))
    return result

def parse_into_list(numbers: str) -> List:
    """Parse a string of numbers into a list"""
    numbers = handle_delimiters(numbers)
    return numbers.split(",")

def handle_delimiters(numbers: str) -> str:
    """
    Handle delimiters according to the `add` function requirements.
    """
    alt_delims = ["\n"]
    if numbers.startswith("//"):
        delims_header = get_delimiters_header(numbers)
        delims = get_delimiters(delims_header)
        for delim in delims:
            alt_delims.append(delim)
        numbers = cleanup_numbers_string(numbers, delims_header)
    for delim in alt_delims:
        numbers = numbers.replace(delim, ",")
    return numbers

def get_delimiters_header(numbers: str) -> str:
    """Helper to handle the `//...\n` header and return the section containing the delimiters"""
    re_delims_header = re.compile(r"""
        //    # match beginning delimiter
        (.*)  # capturing group of 0 or more of any char
        \n    # match ending delimiter
        """, re.VERBOSE)
    return "".join(re.findall(re_delims_header, numbers))

def get_delimiters(delims_header: str) -> List:
    """Helper to handle each individual delimiter and return a list containing them"""
    re_delims = re.compile(r"""
        \[      # match a literal [
        (       # start capturing group
        [^]]*   # match anything except a closing ] 
        )       # close capturing group
        \]      # match a literal ]
        """, re.VERBOSE)
    return re.findall(re_delims, delims_header)

def cleanup_numbers_string(original: str, delims_header: str) -> str:
    """Helper to remove the delimiters header section and return only the delimited list"""
    return original.replace("//{0}\n".format(delims_header), "")
\$\endgroup\$
6
\$\begingroup\$

Personally I think your code is good. I personally would do things slightly differently. However I don't think there are really any major, or minor, problems. However I don't really know testing in Python, and so won't comment on that.

Some things I'd do differently are:

  • I personally don't like erroring whilst handling another error. Even more so when both raise the same error, but with different messages. And so I'd just stick to using int without a try-except.
  • Personally I'd prefer to use a comprehension than a for loop. And so I'd change lines 26 to 37 to three comprehensions.
  • I prefer using sum over manually adding numbers. sum(i for i in numbers if i <= 1000) is easy to read and, IIRC, faster.
  • If I came across parse_into_list in the wild, I'd think it should take a delimiter argument. However that's still in the numbers argument. Instead I'd go for a split_calculator_input function. This would return just numbers. And so I'd use something like the below, which I think is slightly easier to read than your functions.

    def split_calculator_input(input):
        delimiter, numbers = re.match(r'(?s)^(?://\[([^\n]+)\]\n)?(.*)$', input).groups()
        delimiter = '|'.join((delimiter or r',][\n').split(']['))
        return re.split(delimiter, numbers)
    

I had a go at doing this myself, since it's a one-time thing I didn't split the code into a lot of functions, but came up with the following:

def add_string_numbers(input):
    if not input:
        return 0
    delimiter, numbers = re.match(r'(?s)^(?://\[([^\n]+)\]\n)?(.*)$', input).groups()
    delimiter = '|'.join((delimiter or r',][\n').split(']['))
    numbers = [int(i) for i in re.split(delimiter, numbers)]
    negatives = [i for i in numbers if i < 0]
    if negatives:
        raise ValueError('Negatives not allowed {} found'.format(negatives))
    return sum(i for i in numbers if i <= 1000)
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.