Python Calculator Test Driven Development

Question

I am a beginner in TDD and I just coded a Calculator in Python with TDD approach. I would be more than happy to hear your opinions out. And since I am a beginner in TDD, I would love to hear some tips.

Tests :

import Calculator
import unittest


class CalculatorTests(unittest.TestCase):

    def setUp(self):
        self.calculator = Calculator.Calculator()

    def test_add_method(self):
        result = self.calculator.add(4, 2)
        self.assertEqual(6, result)

    def test_add_method_invalid_value(self):
        self.assertRaises(ValueError, self.calculator.add, "four", "five")

    def test_multiply_method(self):
        result = self.calculator.multiply(5, 3)
        self.assertEqual(15, result)

    def test_multiply_method_invalid_value(self):
        self.assertRaises(ValueError, self.calculator.multiply, "four", "five")

    def test_sub_method(self):
        result = self.calculator.sub(6, 4)
        self.assertEqual(2, result)

    def test_sub_method_invalid_value(self):
        self.assertRaises(ValueError, self.calculator.sub, "four", "five")

    def test_div_method(self):
        result = self.calculator.div(5, 1)
        self.assertEqual(5, result)

    def test_div_method_invalid_value(self):
        self.assertRaises(ValueError, self.calculator.div, "five", "four")

    def test_div_method_zero(self):
        self.assertRaises(ZeroDivisionError, self.calculator.div, 5, 0)


if __name__ == '__main__':
    unittest.main()

Calculator.py :

number_types = (int, float, complex)


class Calculator:

    @staticmethod
    def validate_args(x, y):
        if not isinstance(x, number_types) and not isinstance(y, number_types):
            raise ValueError

    def add(self, x, y):
        self.validate_args(x, y)
        return x + y

    def multiply(self, x, y):
        self.validate_args(x, y)
        return x*y

    def sub(self, x, y):
        self.validate_args(x, y)
        return x-y

    def div(self, x, y):
        self.validate_args(x, y)
        return x/y

Answer 1

TDD

It's quite hard to review TDD as it's more of a way to do work than an observable product. What I mean is that whether you did TDD or not, the unit tests would mostly be the same.

One minor thing I'd point out is that the c variable name isn't good. One strong point of TDD/Unit Testing IMO is that it can act as a documentation of your code, so it should be explicit. Name it calculator; you won't be losing much space doing so.

Calculator class

Regarding the class itself, there are two things I'd like to review.

Nesting makes the code hard to read. In your case it's not so bad because methods are very short, but it's a good habit you should take.

Instead of :

if isinstance(x, self.number_types) and isinstance(y, self.number_types):
    return x + y
else:
    raise ValueError

I would do

if not isinstance(x, self.number_types) and not isinstance(y, self.number_types):
    raise ValueError

return x + y

This way if you had to add code in your method, you wouldn't need to hang in a else statement for the whole flow.

Second, it's fine that you throw a ValueError, but I think you should add a message otherwise we can't know why the calculator crashed.

Exceptions

With regards to the comments, I'd like to talk a little bit more about your exception handling. Should it be raised in the UI or in the class itself?

The boring answer would be "It depends". In my opinion, there are advantages to both cases.

• If the exception is raised in UI, it is raised as soon as possible, which means you don't have to work with potentially invalid data in your application flow.

• If the exception is raised in the class itself, you are 100% sure that wherever/however you call your calculator, the input will be validated.

So, what is better? I'd say it's better if the exception is raised in the class, as you'll be sure you won't ever have to repeat your validation if you reuse your calculator elsewhere. But, if I think about the MVC.Net's way of working, for example, it validates the user input on client and server side. This way, there is a quick validation that is made to make sure everything makes sense, then in the server validation (in your case, you class), the input is thoroughly validated then used.

For the sake of "having the best code possible", I'd say do the validation in both and see for yourself what is your gain :)

Answer 2

I'd probably say the same thing as @TopinFrassi with the addition of:

• The type validation should be extracted out to a common method. Any time you have 2 repeating (or 3 depending on your coding mantra) logic blocks you should refactor things to a common method (e.g. validate_args(x, y))
• You're are writing instance methods which in these cases they look more appropriate to be @staticmethods.
• number_types should be a constant. There's no need to have duplicate list for every instance of the object.
• If you're really doing TDD, you must code the tests as if your antagonist was writing the implementation. In your example for add, you have 4 and 2 as args with result 6 for which the simplest implementation would be return 6 and as such your tests are not adequate and you really didn't do "TDD". Generally, using random ints helps here to force the imaginary antagonist to write the expected code.
• It's fine to include multiple asserts in a single test to ensure the function works as expected. I'd probably add multiple asserts in all the op tests.
• Your ValueErrors are tested with both args as invalid which means that the edge case of one invalid arg has undefined behavior and the implementation could be checking the first arg, second arg, or both. I'd add tests for checking extra things there. Given that you use the same logic in all ops, you can make a helper test generator to easily validate all defined functions to reduce duplication in tests too.