5
\$\begingroup\$

I recently read the book clean code by Robert C. Martin and I'm trying to apply his style of writing methods to the FizzBuzz programming example.

So far I have two "public" methods called: start and parseNumber, that respectively start the FizzBuzz program or parse a single number according to the FizzBuzz principle.

class FizzBuzz(object):

    def start(self, end_number):
        return ",".join(self._parse_numbers(end_number))

    def _parse_numbers(self, end_number):
        number_list = []
        for number in range(1, end_number+1):
            number_list.append(self.parse_number(number))
        return number_list

    def parse_number(self, number):
        if self._is_number_divisible_by_five_and_three(number):
            return "FizzBuzz"
        elif self._is_number_divisible_by_three(number):
            return "Fizz"
        elif self._is_number_divisible_by_five(number):
            return "Buzz"
        else:
            return str(number)

    def _is_number_divisible_by_five_and_three(self, number):
        return number % 15 == 0

    def _is_number_divisible_by_three(self, number):
        return number % 3 == 0

    def _is_number_divisible_by_five(self, number):
        return number % 5 == 0

I'm happy with the last 3 methods that are clearly doing one thing. But the _parse_numbers and parse_number methods can be improved, I just don't know how to do that.

\$\endgroup\$
  • 3
    \$\begingroup\$ I can't find anything in the code that does parsing, so using the word "parse" doesn't make sense. \$\endgroup\$ – Matti Virkkunen Nov 3 '15 at 18:01
  • \$\begingroup\$ AFAIK python will let you down whenever you try to apply or learn best practices in OOP, I have got bitten by it many time, if you don't agree just wait and see all the upcoming answers. \$\endgroup\$ – CodeYogi Nov 4 '15 at 7:01
7
\$\begingroup\$

First of all, you don't need to make a class for this. There's no real reason for FizzBuzz to be an object when all you have is a collection of functions anyway. Likewise, you have a lot of functions that don't really need to be functions. All your test functions could just be plain if statements and that'd make them more readable to Python. I see you've discovered the _ naming convention in Python, where names beginning with that are to be considered private. However you don't need to use these that often. Nothing's ever going to be truly private in Python without a lot of work, so instead allow people to access functions if they want to. Only mark functions private if there's actual problems with them attempting to use the function outside of a specific process.

parse_number is good, though I'd replace the tests with plain tests instead of your functions.

def parse_number(number):
    if number % 15 == 0:
        return "FizzBuzz"
    elif number % 3 == 0:
        return "Fizz"
    elif number % 5 == 0:
        return "Buzz"
    else:
        return str(number)

You could also add a docstring and maybe a comment, to indicate what the function does and returns as well as noting how number % 15 == 0 is the same as (number % 5 == 0) and (number % 3 == 0).

def parse_number(number):
    """Returns a string Fizzbuzz representation of a number"""

    # This is the same as (number % 5 == 0) and (number % 3 == 0)
    if number % 15 == 0:

Start seems like a not very worthwhile function and not a great name if you're no longer making a class. But if you want to keep it a name like fizzbuzz_string would be better.

Now, your _parse_numbers isn't a great name because it's too similar to an existing function. There's also little reason to mark it as private. Instead this could be your main method, or at least call it fizzbuzz. You could also build it easier and faster with something called a list comprehension. A list comprehension is basically a for loop like expression that will be evaluated to create a list. Yours is very simple, you just need to run parse_number on each number in your range, so this is how you could write it:

def parse_number(number):
    """Returns a string Fizzbuzz representation of a number"""

    # This is the same as (number % 5 == 0) and (number % 3 == 0)
    if number % 15 == 0:
        return "FizzBuzz"
    elif number % 3 == 0:
        return "Fizz"
    elif number % 5 == 0:
        return "Buzz"
    else:
        return str(number)

def fizzbuzz(end_number):
    """Return a list of Fizzbuzz parsed numbers up to end_number."""

    return [parse_number(number) for number in range(1, end_number+1)]

def fizzbuzz_string(end_number):
    return ",".join(fizzbuzz(end_number))

Now you can easily just call print fizzbuzz_string(number) and get the full list without needing an object or the other extraneous functions.

\$\endgroup\$
  • \$\begingroup\$ This is actually better. to make sure the parse_number is correctly implemented is it an idea to call it parse_fizzbuzz_number(number)? \$\endgroup\$ – Kickstarter Nov 3 '15 at 15:22
  • \$\begingroup\$ @Kickstarter That specificity would make it clearer. It's up to you whether you want to make the name more explicit and longer. \$\endgroup\$ – SuperBiasedMan Nov 3 '15 at 15:29
  • \$\begingroup\$ I don't know why your answer is accepted. I would criticize it as the least flexible solution as it violates SOLID principle in all way it can. \$\endgroup\$ – CodeYogi Nov 4 '15 at 8:04
  • \$\begingroup\$ For an example, you need to modify the parse_number method every time you need to add a new case accompanied by the long list of if cases. \$\endgroup\$ – CodeYogi Nov 4 '15 at 8:06
5
\$\begingroup\$

I'll have to agree with some of the other that I don't see the need for using a class in this case, although you get a little credit for using the naming scheme for public and privat methods.

But what I would focus on is your _is_number_divisible_by_five_and_three() & co methods. You say that you're happy with them as they do just one thing. In my book they are too localised and specific. They are not very reusable, and you would be better served with one general method:

def is_divisible(number, modulo):
    """Return if number % modulo is equal to 0."""

    return number % modulo == 0

This would allow you to simplify your logic in the badly name parse_number() to use is_divisible(number, 3) and so on. Now you have a function with might be useful elsewhere, even though still pretty simple.

As a general principle if you see repeating blocks of code you can:

  • Consider if the blocks can be collated into one single function, like for is_divisible()
  • Consider if the blocks can be converted into a loop of some sorts, like in parse_numbers() where you do the same test (with a different number) and returning a text in all cases

In addition your code calculates the modulo one time too much as you could already know the answer for number % 15. A good way to handle this, minimizing calculations, is to build a list of all fizz buzz words, and then for the final output check if you have values in the list and join them together. If no values in list, return the string version of the number.

Code refactored

Here is an alternate version combining these suggestions, and an extension allowing for additional fizz buzz words:

from collections import namedtuple

def is_divisible(number, modulo):
    """Return if number % modulo is equal to 0."""

    return number % modulo == 0


FizzBuzz = namedtuple('FizzBuzz', 'number, name')

fizz_buzz_words = [
    FizzBuzz( 3, 'Fizz'),
    FizzBuzz( 4, 'Buzz'),
    FizzBuzz( 7, 'Bang'),
    FizzBuzz(11, 'Boom'),
  ]

def to_fizz_buzz_string(n):
    """Returns an extended FizzBuzz string representation of a number."""
    fizz_buzzes = [fb.name for fb in fizz_buzz_words if is_divisible(n, fb.number)]

    return ''.join(fizz_buzzes) if fizz_buzzes else str(n)


for a in range(124, 134):
   print to_fizz_buzz_string(a)

print ', '.join(to_fizz_buzz_string(n) for n in range(923, 935))

At end is two different ways to call the newly created to_fizz_buzz_string() conversion method.

\$\endgroup\$
  • \$\begingroup\$ You have achieved the single point of knowledge (fizz_buzz_words), a very good solution indeed :) \$\endgroup\$ – CodeYogi Nov 4 '15 at 8:07
3
\$\begingroup\$

A Class is for persistent state, here some top level functions would work as well with less code bloat.


When you write:

number_list = [] 
for number in range(1, end_number+1):
     number_list.append(self.parse_number(number))
return number_list

You are just mapping parse_number over a range, so just use map (or a generator expression).

\$\endgroup\$
  • 1
    \$\begingroup\$ I can't speak for the author's style that OP is studying, but as @caridorc alluded to with the generators comment, I think this is a reasonable case to simplify style further with a list comprehension: return [self.parse_number(number) for number in range(1, end_number+1)] \$\endgroup\$ – Taylor Edmiston Nov 3 '15 at 20:13
  • \$\begingroup\$ @tedmiston a generator is like a list comprehension but lazy, so it is evalueted at the last moment taking up no space \$\endgroup\$ – Caridorc Nov 3 '15 at 20:16
  • \$\begingroup\$ I know this, but thought I'd be helpful to point out to OP. Since he wants to print the whole list anyway, I thought it seemed reasonable to mention as an extension to your answer. \$\endgroup\$ – Taylor Edmiston Nov 3 '15 at 21:09
3
\$\begingroup\$

You've got a solid start here, but consider a real world problem (and the common follow up question to Fizz Buzz):

The customer now wants your program to also print "Bang" when the number is a multiple of 7.

So, with your current process, you'd need a couple more if statements (for 7, 21 and 35). Not too hard to do, and you can just copy the code you already have for the most part, so this is doable.

But then a few years go by and the company gets bigger. Now, when the number is a multiple of 11, you need to print "Boom".

So are you going to write out a chain of 24 if statements? What if you mess up a number somewhere? Or put the if conditions in the wrong order, like having the check for 15 before the check for 165? You have an unmanageable mess.

The correct solution is to use a list, pairing each number requirement to its word, and build the "FizzBuzzBangBoom" string as you iterate over the list. The method to add a "word" to the string operates the same no matter what the number input is - it's just a modulo function - so you can cut down on a lot of repetition.

As a challenge, try to repeat your project with less repetition (the DRY principle) and SPECIFICALLY do not have the numbers 3, 5, 7 or 11, or any multiples of them, appear in your code more than once.

\$\endgroup\$
  • \$\begingroup\$ hash map? What are you hashing over? \$\endgroup\$ – Barry Nov 3 '15 at 21:17
  • \$\begingroup\$ I don't know what you mean, hash is not a verb. I'm saying you should have the hash {3: "Fizz", 5: "Buzz", ...} \$\endgroup\$ – Devon Parsons Nov 3 '15 at 21:20
  • \$\begingroup\$ Hash can definitely be used as a verb, but why use a dict here? You just need to iterate over the contents... \$\endgroup\$ – Barry Nov 3 '15 at 21:28
  • 3
    \$\begingroup\$ I'll see your DRY and raise you a YAGNI. \$\endgroup\$ – brian_o Nov 4 '15 at 2:13
  • 2
    \$\begingroup\$ @CodeYogi, Two reasons to use a list in this particular case: 1) You don't do a lookup of a single element, where the dict shines, you simply traverse, 2) The list is ordered, whilst a dict is by default is unordered. And if unordered you could get "BuzzFizz" which wouldn't make sense... :-p \$\endgroup\$ – holroy Nov 4 '15 at 14:21
1
\$\begingroup\$

How about a bit of dependency injection?

# -*- coding: utf-8 -*-
"""A generic configurable FizzBuzz."""

from functools import partial


def byn(number, div, out):
    assert isinstance(number, int)
    assert isinstance(div, int)
    if number % div == 0:
        return out
    return ''


def to_str(number):
    return str(number)


class FizzBuzz(object):

    _actions = (
        partial(byn, div=3, out='fizz'),
        partial(byn, div=5, out='buzz'),
    )

    _default_action = partial(to_str)

    def __init__(self, actions=None, default_action=None):
        if actions:
            self._actions = actions
        if default_action:
            self._default_action = default_action

    def response(self, number):
        rstr = ''
        for action in self._actions:
            rstr += action(number)
        if not rstr:
            rstr = self._default_action(number)
        return rstr

Full code at https://github.com/cleder/FizzBuzz

\$\endgroup\$
  • 1
    \$\begingroup\$ your to_str is not needed. clearer here would be plain str. You can even skip the partial on the default action. I would also find your response clearer as a generator \$\endgroup\$ – Maarten Fabré Apr 25 at 9:27

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.