FizzBuzz in Ruby

Question

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?

• Rainbolt: 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).

Answer 1

• 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}

• user25840: The program was originally built to be code golf, and thus the odd variable names. But I've still learnt something today, thanks!
• Flambino: A PascalCased method name? My Ruby-sense is tingling...
• Schism: It would be fun to extend Integer to hold the divisibleBy3? and divisbleBy5? methods.
• Flambino: Ah, yup, there we go, the very instant I posted my last comment :)
• tokland: I'd move the puts to the each block. More modular, less repetition.

Answer 2

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

• Flambino: @Vogel612 I was unaware that that was a requirement. Where do you see that?
• Vogel612: 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 ..
• Flambino: @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: @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.
• Blorgbeard: 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.

Answer 3

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).

• 200_success: @Vogel612 Not sure what your objection is, or whether you're being serious.
• Vogel612: 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.
• 200_success: @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.
• cHao: @Vogel612: I've seen it. And i feel it's paranoid to assume that a well-known problem is suddenly going to rather radically change its specification. Whatever happened to YAGNI?
• cHao: @Vogel612: Except that the acceptance criteria are met. fizz = 'Fizz'; buzz = 'Buzz'; puts ("#{fizz}#{buzz}" == "FizzBuzz") outputs "true".

Answer 4

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).

• Flambino: +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
• tokland: @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.
• Cary Swoveland: self is required in the definition of divisible_by. An even simpler example is class Integer; def mod(n); % n; end; end. 4.mod(3) => "n\n". Do you know the rule that requires self to be explicit here? (Other rules, for example, are that write accessors must include self. and self.class is required to obtain self's class.)

Answer 5

Here's how I would implement this in ruby, not as a golfing or obscure route, but rather making use of a lot of best practice ruby idioms:

class FizzBuzz
 def initialize(start = 1, last = 100)
 @start = start
 @last = last
 end
 def each
 range.each do |n|
 yield fizzbuzz(n)
 end
 end
 private
 attr_reader :start, :last
 def range
 start..last
 end
 def divisible_by_3(n)
 n % 3 == 0
 end
 def divisible_by_5(n)
 n % 5 == 0
 end
 def divisible_by_15(n)
 n % 15 == 0
 end
 def fizzbuzz(n)
 case
 when divisible_by_15(n)
 'fizzbuzz'
 when divisible_by_3(n)
 'fizz'
 when divisible_by_5(n)
 'buzz'
 else
 n
 end 
 end
end
FizzBuzz.new(1,50).each {|f| puts f}

What's idiomatic here?

• Start with a class. Default values in initializer yields the most basic example.
• Small public API. The rest of the methods are private, to indicate they aren't expected to be used directly.
• private attr_reader reads better than using instance variables directly
• separating the responsibility of output vs the generation of the data. The FizzBuzz class simply iterate over the range and returns the results. The user of the class is responsible for output, hence the each block at the bottom.
• small methods. Each method does one thing, and is named in such a way that you can easily tell what it does. reading the short method is easy to confirm what the method does.
• note that divisible by 3 and divisible by 5 implies divisible by 15. One could make an argument for making that method just use the previous methods: divisible_by_3 && divisible_by_5 but I think think dividing by 15 is better.

Yes, this is significantly longer than your example... But on larger projects with more complicated logic, following these idioms will greatly enhance the readability and thus, maintenance, of your code base.

Another version of the fizzbuzz method could be:

 def fizzbuzz(n)
 divisible_by_15(n) and return 'fizzbuzz'
 divisible_by_3(n) and return 'fizz'
 divisible_by_5(n) and return 'buzz'
 n
 end

This eliminates a supposed code smell of the case statement, but it's debatable whether it's better or not. Some people object to more than one return statement; others object to case statements... I could go either way.

• RubberDuck: I like your use of a class, but I could say a thing or two about using 15 the way you have. Might want to put this up for review yourself. =;)-
• DGM: I noted that in a bullet point - and in this simple example it's easy to say that something divisible by both 3 and 5 is also divisible by 15. If these strategies did something more complex, though, would require reusing the existing 3 and 5 methods. It's just a matter of whether your problem domain allows for that semantic optimization.