Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I have this implementation of the FizzBuzz challenge in Ruby:

(1..100).each do |n|
    i_3=(n%3==0)
    i_5=(n%5==0)
    case
        when i_3&&i_5
            puts 'fizzbuzz'
        when i_3
            puts 'fizz'
        when i_5
            puts 'buzz'
        else
            puts n
    end
end

It prints the numbers and words just as I would expect it to.

Is there a way to make this better follow Ruby best practices?

share|improve this question
    
You may be able to learn a thing or two from one of the four Ruby answers to this question. Disclaimer: No code on that site follows best practice (on purpose). –  Rainbolt Jul 14 at 18:40

4 Answers 4

up vote 11 down vote accepted
  • You've inlined the whole program. It would be beneficial to separate FizzBuzz into it's own method that accepts n as an argument.
  • Speaking of n, I discourage the use of one and two letter variable names. number would be better.
  • I like that you precalculated booleans before entering the select statement, but these two need better names. divisibleBy3 and divisibleBy5 would be better.
  • I also like that you interpreted the requirements correctly and are not concatenating the results in an attempt to prematurely optimize. See this for more info.
  • Some of your statements could use some breathing space. (n % 3) instead of (n%3) etc.
  • Brackets aren't necessary in Ruby, but they help to clarify.
  • As @rofl pointed out in chat, you're printing "fizz", "buzz", and "fizzbuzz" instead of "Fizz", "Buzz", and "FizzBuzz".

Implementing my suggestions, you get something like this.

def fizzbuzz(number)
    divisibleBy3 = (number % 3 == 0)
    divisibleBy5 = (number % 5 == 0)

    case
        when divisibleBy3 && divisibleBy5
            puts "FizzBuzz"
        when divisibleBy3
            puts "Fizz"
        when divisibleBy5
            puts "Buzz"
        else 
            puts number
    end
end

(1..100).each {|n| fizzbuzz n}
share|improve this answer
    
The program was originally built to be code golf, and thus the odd variable names. But I've still learnt something today, thanks! –  Undo Jul 13 at 16:05
1  
A PascalCased method name? My Ruby-sense is tingling... –  Flambino Jul 13 at 16:50
1  
It would be fun to extend Integer to hold the divisibleBy3? and divisbleBy5? methods. –  Schism Jul 13 at 18:07
1  
Ah, yup, there we go, the very instant I posted my last comment :) –  Flambino Jul 13 at 19:03
1  
I'd move the puts to the each block. More modular, less repetition. –  tokland Jul 15 at 8:40

As ckuhn203 pointed out, the variable names aren't great. In this case, I'd consider just calling them fizz and buzz.

Otherwise it's OK. There are so many different ways to do this. The case statement is a good choice for the usage here, but you could also do:

(1..100).each do |number|
  fizz = number % 3 == 0
  buzz = number % 5 == 0
  print "Fizz" if fizz
  print "Buzz" if buzz
  print number if !fizz && !buzz
  print "\n"
end

You could also use print number unless fizz || buzz but using unless with compound conditions can quickly become confusing to read, so I'd rather use good ol' if when anything more than a single boolean expression is involved.

Or use string concatenation

(1..100).each do |number|
  line = ""
  line << "Fizz" if number % 3 == 0
  line << "Buzz" if number % 5 == 0
  puts line.empty? ? number : line
end

Or, if you want a more flexible approach, you could do something like

denominators = { "Fizz" => 3, "Buzz" => 5 } # or more

(1..100).each do |number|
  matches = denominators.map { |name, divisor| name if number % divisor == 0 }
  puts matches.any? ? matches.join : number
end

Note that hashes are unordered in Ruby 1.8 and below, so it won't necessarily work correctly there, possibly printing "BuzzFizz". However, you can just use nested arrays instead to ensure ordering: [["Fizz", 3], ["Buzz", 5]]

And of course, any of these could be wrapped as methods, as ckuhn suggested.

Update: So apparently (see comments) the FizzBuzz task can be construed as printing "Fizz", "Buzz" or "FizzBuzz" as separate, distinct strings without using concatenation. The original problem statement does not, to my eyes, state this. It simply gives the expected output. From there, it's up to you (which is the point of the task, really)

Still, if the actual point is to print 3 distinct strings, then you can do something like

denominators = { "Whatever" => 15, "Fizz" => 3, "Buzz" => 5 }

# since ordering matters, you could just sort the hash (making it an array in process) to
# have the highest denominators first, like so:
# 
#   denominators.sort_by(&:last).reverse

(1..100).each do |number|
  match = denominators.detect { |name, divisor| number % divisor == 0 }
  puts match ? match.first : number
end

But again, I'd argue that the original spec does not require any such thing.


This is just for fun, because Ruby lets you monkey-patch anything. Of course you should not monkey-patch stuff like this "in real-life" - it's a super obnoxious "solution" I've just included for fun.

class Fixnum
  alias_method :original_to_s, :to_s

  def to_s
    str = ""
    str << "Fizz" if self % 3 == 0
    str << "Buzz" if self % 5 == 0
    str.empty? ? original_to_s : str
  end
end

puts (1..100).to_a # to_s gets called automatically

You just can't print integers normally anymore if you do this :-P

share|improve this answer
    
@Vogel612 I was unaware that that was a requirement. Where do you see that? –  Flambino Jul 13 at 23:22
    
Link from ckuhn203s answer: codereview.stackexchange.com/questions/33717/… . You will find a more detailed explanation there.. i am currently on mobile and it's 1:30 here, so please accept my apologies for just posting a link .. –  Vogel612 Jul 13 at 23:25
    
@Vogel612 Hmm... I'd argue that the original (Atwood) spec is unclear then. It just says "For numbers which are multiples of both three and five print 'FizzBuzz'" - nothing more. And that can be achieved just fine with string concatenation. The answer you linked says, in effect, "what if the spec was different?" - or at any rate "what if we read something into the spec?". So it's not the same problem. Still, it's easy enough to do, and I'll add a line, but I call shenanigans. –  Flambino Jul 13 at 23:39
3  
@ckuhn203 Agreed, I do believe it's intentionally vague, since interpretation is also up to the coder. And in, say, an interview situation, I'd find it perfectly fine to solve the problem as you read it, and then be told "ok, but what if...?". Adaptability (on the coder's or the code's part) to changing requirements is a good thing to examine. But just giving vague requirements and then faulting a valid solution is shenanigans. Too easy to turn it around and fault it for not using concatenation (or whatever) if it isn't. Damned if you do, damned if you don't. –  Flambino Jul 14 at 1:27
3  
The point of fizzbuzz is to see whether a candidate can write a simple program. There is no point trying to optimize it, or make it more extensible. That's missing the point. If you can get a fizzbuzz program to compile, run, and print the correct output, you WIN fizzbuzz. Now go write a real program. –  Blorgbeard Jul 14 at 3:35

I'd prefer to see counting loops written as 1.upto(100) do … end.

Ruby case blocks are expressions. The puts can be factored out:

1.upto(100) do |n|
  i_3 = (n % 3 == 0)
  i_5 = (n % 5 == 0)
  puts case
    when i_3 && i_5
      'fizzbuzz'
    when i_3
      'fizz'
    when i_5
      'buzz'
    else
      n
  end
end

Personally, I'd go further: instead of treating i_3 and i_5 as booleans, assign them a noise.

1.upto(100) do |n|
  fizz = (n % 3 == 0) ? 'Fizz' : nil
  buzz = (n % 5 == 0) ? 'Buzz' : nil
  puts case
    when fizz || buzz
      "#{fizz}#{buzz}"
    else
      n
  end
end

Or, replace case with a ternary expression:

1.upto(100) do |n|
  fizz = (n % 3 == 0) ? 'Fizz' : nil
  buzz = (n % 5 == 0) ? 'Buzz' : nil
  puts (fizz || buzz) ? "#{fizz}#{buzz}" : n
end

To enhance code reusability, I suggest putting the code into a function, and yielding the results instead of printing them directly.

def fizzbuzz(max=100)
  1.upto(max) do |n|
    fizz = (n % 3 == 0) ? 'Fizz' : nil
    buzz = (n % 5 == 0) ? 'Buzz' : nil
    yield (fizz || buzz) ? "#{fizz}#{buzz}" : n
  end
end

fizzbuzz { |fb| puts fb }

Note the Ruby whitespace conventions: two spaces of indentation (you used four), and some space on each side of binary operators (you used none).

share|improve this answer
    
Same as with Flambino's answer, you answer does not fulfil the actual requirements in the 2 later codesamples :( –  Vogel612 Jul 13 at 23:14
1  
@Vogel612 Not sure what your objection is, or whether you're being serious. –  200_success Jul 13 at 23:44
1  
A little late, but here goes. The point is, that the "FizzBuzz" part should be a separate if-condition. In this answer ChrisWue elaborates, that, as soon as you change the output for the FizzBuzz condition, you'd need to refactor the whole program. The actual requirement is: For numbers divisible by both 3 AND 5, print "FizzBuzz". What you do is: For numbers divisible by 3 AND 5, print "Fizz" and then "Buzz". and that is a horse of different color. –  Vogel612 Jul 14 at 8:55
3  
@Vogel612 Your concern is misplaced, I think. The output is identical and correct. Should the requirements change, it would be trivial to modify the code accordingly. –  200_success Jul 14 at 10:10
    
Let's agree to disagree. I can accept that it may be nitpicking in the context of a FizzBuzz itself, but all else equal it's a violation of the acceptance criteria IMO :( –  Vogel612 Jul 14 at 10:21

I cannot add anything new to the comments of other users regarding your code. But striving for the most declarative code I can think of, I'd write:

class Integer
  def divisible_by?(n)
    (self % n).zero?
  end
end

1.upto(100) do |n| 
  string = case
    when n.divisible_by?(3) && n.divisible_by?(5) then 'fizzbuzz'
    when n.divisible_by?(3) then 'fizz'
    when n.divisible_by?(5) then 'buzz'
    else n.to_s
  end

  puts(string)
end

(Well, actually I think strings = 1.upto(100).lazy.map { ... } + print_lines(strings) would be still more declarative, but this style is not -yet- idiomatic in Ruby).

share|improve this answer
2  
+1 for monkey patching Integer and using .zero?. –  ckuhn203 Jul 14 at 11:08
1  
+1 for the divisible_by? method from me too. Was considering the same thing (actually, I went looking for such a method in a while back, figuring it might already be built in, but nope). Don't know if I'd find the .lazy.map construction more declarative, compared to a simple each, though –  Flambino Jul 14 at 11:55
1  
@Flambino. Yes, it's not clear it would be better. But I think it would be more modular, at least (and more functional). First, you get the result (lines). Now what do you want? printing them? ok, then print_lines(lines) or whatever. –  tokland Jul 14 at 11:57

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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