Beginner FizzBuzz in Python

Specific feedback on performance would be appreciated.

There are some things to improve performance-wise:

• on every single iteration, you are calculating all the 3 remainders, even though you need a single one (in the best case)
• the 0 in fizzBuzzValues check is not really needed (and the "in list" is O(n) in general, even though n=3 in this case, but you are doing that in a loop) - you can compare each of the remainders one by one
• if you can calculate something before the loop - always do it - the fizzNumber * buzzNumber can be defined before the loop

Modified code:

def fizz_buzz_new(fizz_number, buzz_number):
    fizz_buzz_number = fizz_number * buzz_number
    for i in range(1, 101):
        if i % fizz_buzz_number == 0:
            print("Fizzbuzz")
        elif i % fizz_number == 0:
            print("Fizz")
        elif i % buzz_number == 0:
            print("Buzz")
        else:
            print(i)

Some timeit tests (replaced print() with yield to avoid seeing impact of printing to stdout):

$ ipython3 -i test.py 

In [1]: %timeit for x in range(100): list(fizzBuzz(3, 5))
100 loops, best of 3: 3.52 ms per loop

In [2]: %timeit for x in range(100): list(fizz_buzz_new(3, 5))
100 loops, best of 3: 1.98 ms per loop

If you are going to support arbitrary fizzNumber and buzzNumber, then you should do it correctly. For fizzBuzz(3, 6), you would print "Fizz" for 6 instead of "FizzBuzz" as I would expect.

Note that interCaps is not recommended by PEP 8.

Specifics on unneeded code:

Setting a variable just before using it in a loop is a no-op:

  i = 0

Pre-calculating your fizzbuzz values, might be useful if the calculations were expensive, or the results were used multiple times. But in this case, the values are not expensive to calculate, and are only needed once:

  fizzBuzzValues = [i%fizzNumber, i%buzzNumber, i%(fizzNumber * buzzNumber)]

Testing if any value is zero, might be useful, except if any of the values are zero, the stacked if will test them all for zero again:

      if(0 in fizzBuzzValues):

Since you previously checked if any of the values were zero, there is no way to get to this else:

          else:
              break

Removing non-pythonic and uneeded code and formatting to pep8:

def fizz_buzz(fizz_number, buzz_number):

    for i in range(1, 101):
        if i % (fizz_number * buzz_number) == 0:
            print("Fizzbuzz")
        elif i % fizz_number == 0:
            print("Fizz")
        elif i % buzz_number == 0:
            print("Buzz")
        else:
            print(i)

fizz_buzz(3, 5)

To add to the existing answers, one of the principles of functions and functional programming is that a function should have one single responsibility. Right now your function does the following things:

1. Create and populate a list with modulus calculations;

2. Iterate 100 times over the list;

3. Associate results with a value (Fizz, Buzz, FizzBuzz or a number);

4. Print results to the console.

If I were to break down the function, I would make one function that simply checks for Fizz/Buzz and return the appropriate string, or None (a.k.a null) when there is no match, given two numbers. You could also give it optional string arguments to make it able to return other values besides Fizz and Buzz.

def get_fizz_buzz(target_num,  fizz_num, buzz_num, fizz_word="Fizz", buzz_word="Buzz"):
    result = None
    # calculations...
    return result

Then you call either get_fizz_buzz(i, 3, 5) or get_fizz_buzz(i, 3, 5, "Hello", "World") from a loop, and check if the return is None then you know it's not a FizzBuzz number.

For the iteration and printing parts, you could either run that in your main function, or make a function that takes numbers as input then calls the above function and prints the correct result.

Others have pointed out that you could do away with the list part, but if you needed the results in a list for some reason, I'd make a separate function that returns a list, then you can do whatever you need to with the list after it returns.

print is slow, like really slow. Instead it's faster to combine a list, with say '\n'.join, and print it once. It incurs \$O(n)\$ rather than \$O(1)\$ memory however.

Here's a quick timing:

>>> from timeit import timeit
>>> timeit('print("\\n".join(arr))', 'arr = list(map(str, range(100)))', number=10)
0.11
>>> timeit('for n in arr: print(n)', 'arr = list(map(str, range(100)))', number=10)
3.79

And so to speed up your code, all you need to do is minimize the amount of prints. You can do this by building a list like we did in the setup above. Including some of the other changes suggested by other users you can get:

from math import gcd

def _fizz_buzz(fizz, buzz):
    fizzbuzz = gcd(fizz, buzz)
    for i in range(1, 101):
        if i % fizzbuzz == 0:
            yield 'fizzbuzz'
        elif i % fizz == 0:
            yield 'fizz'
        elif i % buzz == 0:
            yield 'buzz'
        else:
            yield str(i)

def fizz_buzz(fizz, buzz):
    print('\n'.join(_fizz_buzz(fizz, buzz)))

fizzBuzz(3, 5)

If you wait on outputting a newline character until after you fizz, buzz, or both, you can very slightly reduce the logical overhead of each iteration. I have no idea what the performance cost is for making multiple output calls instead of making one larger output call, so this could actually make the performance worse in practice.

In the case of a FizzBuzz, Fizz is written with no newline, then Buzz is also written with no newline. After both Fizz and Buzz are printed, a newline is printed. The output of this is equivalent to printing FizzBuzz with a newline all at once.

import sys


def fizz_buzz(fizz, buzz):
    for i in range(1, 16):
        wrote = False
        if i % fizz == 0:
            wrote = True
            sys.stdout.write("Fizz")
        if i % buzz == 0:
            wrote = True
            sys.stdout.write("Buzz")
        if wrote:
            sys.stdout.write('\n')
        else:
            sys.stdout.write(str(i) + '\n');

    sys.stdout.flush()

fizz_buzz(3, 5)

Performance isn't an issue. This is FizzBuzz. Let's take a look at good practice:

  i = 0

This is neccessary in C if you want to see i after the loop, but not in Python. In Python, you have

• function scope - each variable declared in the function is visible afterwards in the function, everywhere.
• implicit binding in for - for i in x creates binding for i if there isn't one, or overwrites if there is.

fizzBuzzValues = [i%fizzNumber, i%buzzNumber, i%(fizzNumber * buzzNumber)]

First of all, you don't exactly need to all of these three at once, but there's other problem in here.

You define only one variable. Why? Having three variables would be better in the practical application, since it improves debugability. Also,

else:
    break

is unneccessary.Let's look at refactored code:

def fizz_buzz(fizz, buzz):
    for i in range(1, 101):
        do_i_fizz = i % fizz
        do_i_buzz = i % buzz
        if(do_i_fizz == 0 and do_i_buzz == 0):
            print("Fizzbuzz")
        elif(do_i_fizz == 0):
            print("Fizz")
        elif(do_i_buzz == 0):
            print("Buzz")
        else:
            print(i)

fizz_buzz(3, 5)

Not that much of a change. That's why you should try learning on something more complicated than that :)

I've always been pretty weak at algorithms, so I'm trying to improve. Specific feedback on performance would be appreciated.

The thing is FizzBuzz is not a problem that shows your algorithms knowledge, I've seen lots of solutions and never seen one done in O(n^2). I mean if you will think about it, you might find a solution in O(n^2) but most obvious solutions will still be O(n). So again this problem is not about algorithms.

The main goal of FizzBuzz is just to show your basic knowledge on specific programming language since it contains all the main operations (loop, output and number operations). It's also a good thing to have a basic image on candidate coding style.

If I would be asked to solve a fizzbuzz I will do something like this:

def real_fizz_buzz(fizz=3, buzz=5):
    print(*('Fizz'*(not i % fizz)+'Buzz'*(not i % buzz) or i for i in range(1, 101)), sep='\n')

Or this:

print(*(map(lambda x: 'Fizz'*(not x % fizz)+'Buzz'*(not x % buzz) or x, range(1, 101))), end='\n')

Or using F literal:

print(*map(lambda x: f"{'Fizz' * (not x%3)}{'Buzz' * (not x%5)}" or x, range(1,101)), sep='\n')

I know this is something that is not easy to read for a beginner, but I just like that python allows you to solve that simple problem in one line and I would use it if I would be asked.

However, my personal opinion that is long as a candidate can solve this problem and does it in O(n), it's fine and acceptable.

So for this particular problem just use any solution that is easier for you to write and understand.