5
\$\begingroup\$

I'm new to Python and wondering what can be improved with the following super novice program - in terms of the solution and code style etc.

test_fizzbuzz.py

import unittest
import fizzbuzz


class TestFizzbuzzFunctions(unittest.TestCase):

    def test_it_converts_multiples_of_three_to_fizz(self):
        self.assertEqual(fizzbuzz.process_number(6), 'fizz')

    def test_it_converts_multiples_of_five_to_buzz(self):
        self.assertEqual(fizzbuzz.process_number(10), 'buzz')

    def test_it_converts_multiples_of_three_and_five_to_fizzbuzz(self):
        self.assertEqual(fizzbuzz.process_number(15), 'fizzbuzz')

    def test_it_does_not_convert_numbers_that_are_not_multiples_of_three_or_five(self):
        self.assertEqual(fizzbuzz.process_number(11), 11)

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

fizzbuzz.py

def process_number(number):
    result = ''
    if number % 3 == 0:
        result += 'fizz'
    if number % 5 == 0:
        result += 'buzz'

    return result or number
\$\endgroup\$
2
\$\begingroup\$

This does not appear to be a true FizzBuzz program. A FizzBuzz program, according to the tag wiki, iterates through the numbers 1-100 and prints the values according to the rules you have.

One thing you did correctly is this:

if __name__ == '__main__':

Many beginners fail to do this. Also, you used snake_case, which is Python standard too.

One thing I found slightly difficult to understand is this:

return result or number

This would not compile in many languages, and it also allows the method to have two return types - a string and an int. I would adjust this to use an if statement and convert the number to a string:

if result != '':
    return result

return str(number)

This way, the method always returns a string.

Because this is just FizzBuzz, if you are going to create a second method for handling it at all, I would just print from within the function, then you do not need to worry about return type:

def process_number(number):
    result = ''
    if number % 3 == 0:
        result += 'fizz'
    if number % 5 == 0:
        result += 'buzz'

    if result != '':
        print(result)
    else:
        print(number)
\$\endgroup\$
  • \$\begingroup\$ Even if it works, using is to compare to the empty string seems a dangerous business. PEP 8 actually recommends going with if not result to check empty sequences, which include strings. I kind of like the OP's approach though, even if it is a little too clever. \$\endgroup\$ – Jaime Feb 17 '15 at 18:45
  • \$\begingroup\$ @Jaime So my if should be if result is not '':? \$\endgroup\$ – Hosch250 Feb 17 '15 at 18:49
  • \$\begingroup\$ No, it should either be if result != '': which doesn't require the empty strings to be the exact same object, just have the same content, or the preferred way to check for an empty string is if not result:. Take a look at the Programming recommendations section in PEP 8. \$\endgroup\$ – Jaime Feb 17 '15 at 20:01
  • 1
    \$\begingroup\$ I downvoted, suggesting the mix of algorithm and Input/Output is terrible. \$\endgroup\$ – Caridorc Feb 17 '15 at 20:12
  • \$\begingroup\$ @Caridorc I normally wouldn't do this, but it is a FizzBuzz, and the algorithm is just for determining the output, it is not really a calculation. \$\endgroup\$ – Hosch250 Feb 17 '15 at 20:41

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.