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 '14 at 18:40

5 Answers 5

up vote 17 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 '14 at 16:05
3  
A PascalCased method name? My Ruby-sense is tingling... –  Flambino Jul 13 '14 at 16:50
3  
It would be fun to extend Integer to hold the divisibleBy3? and divisbleBy5? methods. –  Schism Jul 13 '14 at 18:07
1  
Ah, yup, there we go, the very instant I posted my last comment :) –  Flambino Jul 13 '14 at 19:03
3  
I'd move the puts to the each block. More modular, less repetition. –  tokland Jul 15 '14 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 '14 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 '14 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 '14 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 '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 '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
1  
@Vogel612 Not sure what your objection is, or whether you're being serious. –  200_success Jul 13 '14 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 '14 at 8:55
4  
@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 '14 at 10:10
1  
@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 Jul 14 '14 at 13:06
1  
@Vogel612: Except that the acceptance criteria are met. fizz = 'Fizz'; buzz = 'Buzz'; puts ("#{fizz}#{buzz}" == "FizzBuzz") outputs "true". –  cHao Jul 14 '14 at 14:22

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?. –  RubberDuck Jul 14 '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 '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 '14 at 11:57
    
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.) –  Cary Swoveland Dec 27 '14 at 20:22

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.

share|improve this answer
    
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. =;)- –  RubberDuck Dec 27 '14 at 3:03
1  
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. –  DGM Dec 27 '14 at 5:24

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.