3
\$\begingroup\$

After watching this video by Tom Scott, I decided to make a program that follows that idea. This is what I came up with:

# If a number is divisible by the key, add the value
RULES = {
    3: "Fizz",
    5: "Buzz"
}


def is_divisible(num: int) -> list:
    output = []
    for divider in RULES:
        if (num % divider) == 0:
            output.append(RULES[divider])
    return output


for num in range(100):
    output = is_divisible(num + 1)  # Increment num by 1 so it is 1-100 not 0-99
    if not output:
        print(num + 1)
    else:
        print("".join(output))

My whole idea behind the way I made this is because I wanted it to be as expandable as possible. That way if you wanted to change the code to do something different (like the examples at the end of the above video), you could just add it. I would like to know if there is anything I could do to improve its changeableness and if there are any glaring problems that when fixed doesn't affect the changeability of the code.

\$\endgroup\$
1
  • \$\begingroup\$ def is_divisible(n: int) -> list: return [RULES[d] for d in RULES if n % d == 0] :) \$\endgroup\$
    – code_dredd
    Feb 28, 2020 at 20:18

5 Answers 5

6
\$\begingroup\$

When order of RULES matter, prefer a list of tuples instead of a dict as a dict's order might change depending of the version. You can iterate through the tuple with for divider, value in RULES

If all values have to be capitalized, use str.capitalize in case you don't provide proper capitalization by mistake.

Prefer for num in range(1, 100 + 1): instead of using num + 1 each time. It's really easy to forget the + 1 and num + 1 is confusing.

As @RayButterworth mentioned, is_divisible is a bit awkward. You aren't exactly checking if a number is divisible. You are checking the rules. So use a name like check_rules

Instead of using "".join(output) each time you use check_rules, convert output to a string inside the check_rules function.

This can be done using the idea @AJNeufeld mentioned.

return "".join(value for divider, value in RULES if num % divider == 0)

Use all the parts you wouldn't want to run if imported inside a if __name__ == '__main__' statement.

output = check_rules(num)

if not output:
    print(num)
else:
    print(output)

This part can be a bit simplified with

output = check_rules(num)

if not output:
    output = num

print(output)

Here's the final code:

# The case is weird to emphasize the use of str.capitalize
RULES = [
    (3, "FiZz"),
    (5, "buzz"),
    (7, "MUZZ")
]


def check_rules(num: int) -> str:
    return "".join(value.capitalize() for divider, value in RULES if num % divider == 0)


if __name__ == '__main__':
    END = 120

    for num in range(1, END + 1):
        output = check_rules(num)

        if not output:
            output = num

        print(output)
\$\endgroup\$
8
\$\begingroup\$

You do not provide a testable function that performs the task of returning the desired answer for a specific test case. It is the 'normal number' handling that you intermangled with your test code. The main structure of your program structure should look somewhat like

def fizzbuzz(int):
    #[...]
    return answer

for num in range(1, 101):
    print(fizzbuzz(num))

You are free to do helper functions but you should always provide a complete testable solution. Completely separate the functionality you want to provide from the tests. even split into different files.

\$\endgroup\$
1
  • 1
    \$\begingroup\$ This is actually the best answer, and just got my vote. It targets the most fundamental problem in the original code, which the rest of our answers missed. The only addition I'd make is to give it two parameters, fizbuzz(num, rules), providing an even more general function that could be used in the same program with different sets of rules. \$\endgroup\$ Feb 28, 2020 at 14:29
3
\$\begingroup\$

The name is_divisible could be better.
As is, the leading is_ makes it look like something that you would use as a test.
e.g. if is_divisible(…):


Why is output a list? Wouldn't a string be easier?


The multiple num +1 looks awkward.

Normally I'd suggest assigning the result to another variable, but in this case the problem is with the range(100). Instead, say range(1,101), and then use just num.

\$\endgroup\$
3
\$\begingroup\$

In addition to Ray Butterworth’s comments:

The parenthesis are unnecessary in if (num % divider) == 0:

The is_divisible function has too much code. Creating a list, then appending to the list in a loop, is an antipattern in Python. Python uses list comprehension to simplify and speed up this operation:

def is_divisible(num: int) -> list:
    return [value for divider, value in RULES.items() if num % divider == 0]
\$\endgroup\$
1
  • \$\begingroup\$ +1 for list comprehension. It's valuable to have language-agnostic points in answers, but it's also valuable to encourage mastery of the features specific to whichever language is at hand. \$\endgroup\$
    – J.G.
    Feb 28, 2020 at 16:03
0
\$\begingroup\$

Further to @AJNeufeld's answer, I'll show another Python trick (an empty string is Falsy so '' or 4 is 4), and I suggest you use a list of tuples instead of a dictionary, since its order is guaranteed. All you need is

RULES = [(3, 'Fizz'), (5, 'Buzz')]

def fizzbuzz(n):
    output = ''.join([value for factor, value in RULES if n%factor==0])
    return output or n

for n in range(1, 101): print(fizzbuzz(n))
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.