4
\$\begingroup\$

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
\$\endgroup\$
8
\$\begingroup\$

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 :)

\$\endgroup\$
  • \$\begingroup\$ I wish you remove this statement: whether you did TDD or not, the unit tests would mostly be the same because TDD (if well done) impacts a lot both your code and your unit tests, that is why it is praised \$\endgroup\$ – Billal Begueradj May 7 '18 at 13:44
  • 1
    \$\begingroup\$ @Begueradj I agree with your regarding TDD's advantages. But it doesn't change the fact that I could write the exact same code and then write my unit tests. I understand your POV though, but I think my point stands. \$\endgroup\$ – IEatBagels May 7 '18 at 13:56
  • \$\begingroup\$ Thanks a lot for your review. I get your point. About the error message, do you think is it necessary in this layer of the software or should it be in UI layer ? \$\endgroup\$ – MeteHan May 7 '18 at 15:48
  • \$\begingroup\$ @M.Han It is more than fine to throw exceptions in any layers. The exception belongs there :) \$\endgroup\$ – IEatBagels May 7 '18 at 15:51
  • 1
    \$\begingroup\$ No I mean you said that I need to return a message like "your inputs are wrong". Should I do that in this layer? \$\endgroup\$ – MeteHan May 7 '18 at 18:53
6
\$\begingroup\$

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.
\$\endgroup\$
  • \$\begingroup\$ If one of the args is invalid it will still raise the error because of the "and" in "if" statment. Should I write a common method and call that method from every function, is that what you mean? And thank you for your review, I really appreciate it \$\endgroup\$ – MeteHan May 7 '18 at 19:49
  • \$\begingroup\$ Yeah calling the validation method from every operation is what I meant. As for the raising the error, your tests are not checking the "and" part of the statement in any of the operations. \$\endgroup\$ – Srdjan Grubor May 7 '18 at 20:02
  • \$\begingroup\$ Oh I get what you mean now. And about using random ints, should I generate random ints in every testcase or should I write a common method for generating 2 random ints too ? And is it okay writing a non-test method in Test class? \$\endgroup\$ – MeteHan May 7 '18 at 20:14
  • 1
    \$\begingroup\$ Re. the antagonist -- you might also consider cases like 5 + NaNand division by zero. \$\endgroup\$ – mrblewog May 9 '18 at 8:56
  • \$\begingroup\$ @M.Han no need, you can just have something like x = random.random(), and y = random.random() and you assert with self.assertEquals(x + y) (you can't really assume that the built-in op + is bad). \$\endgroup\$ – Srdjan Grubor May 9 '18 at 11:21

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.