22
\$\begingroup\$

I just started programming in Python this morning, and it is (more or less) my first programming language. I've done a bit of programming before, but never really did much except for "Hello World" in a few languages. I searched around for some Python FizzBuzz solutions, and they all seem significantly more complicated then mine, so I think I must be missing something, even though it works correctly. Could you guys point out any errors I've made, or things I can improve?

count = 0
while (count < 101):
    if (count % 5) == 0 and (count % 3) == 0:
        print "FizzBuzz"
        count = count +1
    elif (count % 3) == 0:
        print "Fizz"
        count = count + 1
    elif (count % 5) == 0:
        print "Buzz"
        count = count +1
    else:
        print count
        count = count + 1
\$\endgroup\$
  • 3
    \$\begingroup\$ Only slightly related to your question but this pages contains a few tricks quite interesting if you are starting to learn Python : reddit.com/r/Python/comments/19dir2/… . One of them leads to a pretty concise (and quite obscure) solution of the Fizzbuzz problem : ['Fizz'*(not i%3) + 'Buzz'*(not i%5) or i for i in range(1, 100)]. \$\endgroup\$ – SylvainD Mar 1 '13 at 9:06
  • \$\begingroup\$ Nice solution! To actually print the items one per line, as most Fizzbuzz questions require, not just show a list of results, this needs to be: print '\n'.join(['Fizz'*(not i%3) + 'Buzz'*(not i%5) or str(i) for i in range(1, 101)]). Also note the 101 - range(1, 100) returns 1 to 99. \$\endgroup\$ – RichVel Mar 21 '17 at 16:45
45
\$\begingroup\$

Lose the useless brackets

This:

while (count < 101):

can just be:

while count < 101:

Increment out of the ifs

Wouldn't be easier to do:

count = 0
while count < 101:
    if count % 5 == 0 and count % 3 == 0:
        print "FizzBuzz"
    elif count % 3 == 0:
        print "Fizz"
    elif count % 5 == 0:
        print "Buzz"
    else:
        print count

    count = count + 1    # this will get executed every loop

A for loop will be better

for num in xrange(1,101):
    if num % 5 == 0 and num % 3 == 0:
        print "FizzBuzz"
    elif num % 3 == 0:
        print "Fizz"
    elif num % 5 == 0:
        print "Buzz"
    else:
        print num

I've also renamed count to num since it doesn't count much, is just a number between 1 and 100.

Let's use only one print

Why do 4 different print, when what is really changing is the printed message?

for num in xrange(1,101):
    if num % 5 == 0 and num % 3 == 0:
        msg = "FizzBuzz"
    elif num % 3 == 0:
        msg = "Fizz"
    elif num % 5 == 0:
        msg = "Buzz"
    else:
        msg = str(num)
    print msg

Light bulb!

"FizzBuzz" is the same of "Fizz" + "Buzz".

Let's try this one:

for num in xrange(1,101):
    msg = ''
    if num % 3 == 0:
        msg += 'Fizz'
    if num % 5 == 0:       # no more elif
        msg += 'Buzz'
    if not msg:      # check if msg is an empty string
        msg += str(num)
    print msg

Copy and paste this last piece of code here to see what it does.

Python is a very flexible and powerful language, so I'm sure there could be other hundred and one different possible solutions to this problem :)

Edit: Improve more

There's still something "quite not right" with these lines:

if not msg:
    msg += str(num)

IMHO it would be better to do:

for num in xrange(1,101):
    msg = ''
    if num % 3 == 0:
        msg += 'Fizz'
    if num % 5 == 0:
        msg += 'Buzz'
    print msg or num

There! Now with:

print msg or num

is clear that num is the default value to be printed.

\$\endgroup\$
  • \$\begingroup\$ Let's use only one print would work well without converting num to a string. It's as if you were already anticipating the need for the conversion on Light bulb!? Or maybe it was the purity of keeping msg a string? \$\endgroup\$ – tshepang Mar 6 '12 at 18:04
  • 2
    \$\begingroup\$ @Tshepang: yeah I know, but it would have been more strange to have a msg variable that one time is a str and another an int. Also it isn't a very good practice to have a variable that could have different types. So I'd say that is the following of the good practices that will lead you towards the light bulb :) [Edit: Yes, I'd like to think it was number 2.] \$\endgroup\$ – Rik Poggi Mar 6 '12 at 18:11
  • 1
    \$\begingroup\$ You reach the best solution about half way through this post and then start making it cuter and less readable. \$\endgroup\$ – Mike Graham Jun 11 '14 at 0:24
  • 4
    \$\begingroup\$ @MikeGraham I disagree. I think the last version is very readable. Honestly I think appending Buzz to Fizz when n is divisible by 3 and 5 is part of the reason it's in the question in the first place, written as it is. The print msg or num is very pythonic in terms of "do this otherwise use the fallback". If it were another language you might explicitly set the output for msg. \$\endgroup\$ – Jordan Reiter May 7 '15 at 13:44
  • 2
    \$\begingroup\$ We'll just have to agree to disagree. If I were interviewing, I'd immediately follow with: "I've changed my mind. I want 7 instead of 5, and instead of Fizz I want Fooz". In the loop solution, you have to rewrite 5 lines of code; in the "custesy" solution, you only have to rewrite 2 lines of code. The "straightforward" solution includes repeating yourself: you check whether something is divisible by 5 twice and you print "Fizz" (on its own or as part of another word) twice. Can we at least agree that the "Let's use only one print" is far better then calling print numerous times? \$\endgroup\$ – Jordan Reiter May 18 '15 at 16:16
7
\$\begingroup\$

Your code looks fine. It certainly works. The only thing I'd say about it is that you are repeating the incrementation in all if blocks. You could just move that out of and after them and you'll achieve the same thing.

if (count % 5) == 0 and (count % 3) == 0:
    print "FizzBuzz"
elif (count % 3) == 0:
    print "Fizz"
elif (count % 5) == 0:
    print "Buzz"
else:
    print count
count = count + 1

You could also condense the if conditions a bit if you recognize that values of 0 are considered False and non-zero is True. You're testing if they're equal to 0 so you could just write it as

if not(count % 5) and not(count % 3):
    print "FizzBuzz"

I would however not do your loop like that using a while loop but instead use a for loop. Then you wouldn't even need the increment statement there.

for count in range(0, 101):
    if not(count % 5) and not(count % 3):
        print "FizzBuzz"
    elif not(count % 3):
        print "Fizz"
    elif not(count % 5):
        print "Buzz"
    else:
        print count
\$\endgroup\$
  • \$\begingroup\$ Thanks for the help! I'll have to play around with this! \$\endgroup\$ – bckwth Mar 6 '12 at 14:53
  • 1
    \$\begingroup\$ Also one thing I'll include here, your first condition could be replaced checking if the number is a multiple of 15 instead of 5 and 3 separately. But what you have is still perfectly fine as they're both logically equivalent. \$\endgroup\$ – Jeff Mercado Mar 6 '12 at 14:56
  • \$\begingroup\$ One quick question that is slightly off-topic: aside from reducing lines-of-code, is there a reason you prefer the 'for' loop rather then my 'while'? Do you find it more readable? Would most experienced programmers prefer your method as more readable? I only ask because to me it seems harder to read, but that could be because it contains concepts I haven't covered yet. \$\endgroup\$ – bckwth Mar 6 '12 at 15:31
  • \$\begingroup\$ Yes it is indeed more readable IMHO. Any programmer should know what a for loop is and what it's used for. Given some variable, the body of the loop will be run for every value in a given collection. A while loop isn't quite as clear. Given some condition, the body of the loop will be run as long as the condition is met. It isn't as clear whether that condition will be met as it can be affected by a number of factors. It could change at any point within the body. With a for loop, there is only one possible way for the "condition" to change and that is it reaches the end of the body. \$\endgroup\$ – Jeff Mercado Mar 6 '12 at 16:37

protected by Jamal May 14 '14 at 5:41

Thank you for your interest in this question. Because it has attracted low-quality or spam answers that had to be removed, posting an answer now requires 10 reputation on this site (the association bonus does not count).

Would you like to answer one of these unanswered questions instead?

Not the answer you're looking for? Browse other questions tagged or ask your own question.