8
\$\begingroup\$

As a casual Rubyist I am mainly interested, how ideomatic my solution is.

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

def fizzbuzz(upper_bound)
  1.upto(upper_bound).map do |number| 
    next "fizzbuzz" if number.divisible_by? 3 and number.divisible_by? 5
    next "fizz" if number.divisible_by? 3
    next "buzz" if number.divisible_by? 5
    next number
  end
end

puts fizzbuzz 100

Remarks: I borrowed the idea for monkeypatching Integer from @tokland here

\$\endgroup\$
6
\$\begingroup\$

If you're aiming for readability rather than efficiency or maintainability — and that is a reasonable tradeoff for FizzBuzz — then I would say "Well done." (You could test for divisibility by 15 for efficiency and avoid monkey-patching for maintainability.)

Conventionally, the output should be one entry per line, rather than an array.

If you're going to take an upper bound as a parameter, you may as well take a range, and the parameter should default to 1..100.

Therefore, a better way to call the code would be

fizzbuzz(1..100).each { |output| puts output }

or, using the default,

fizzbuzz.each { |output| puts output }
\$\endgroup\$
4
\$\begingroup\$

Since we have hardcoded the divisor as 3 and 5, we could do this:

next "fizzbuzz" if number.divisible_by? 15

If you wish to go by the fact that your program should dictate the problem as it is, we could use the following:

  • use a flag: isFizz = (num%3 == 0)
  • use a flag: isBuzz = (num%5 == 0)
  • use the boolean to decide the output.

Analyse the number of times the modulo operation is performed here:

// for n numbers
next "fizzbuzz" if number.divisible_by? 3 and number.divisible_by? 5

100 times (once for each number) + 33 times (if number.divisible_by? 3 will be true 33 times and thus the second operation will be performed)

next "fizz" if number.divisible_by? 3

fizzbuzz above will be false 94 times, so this will happen 94 times.

next "buzz" if number.divisible_by? 5

fizz above will be false 67 times, so this will happen 67 times.

Total = 100 + 33 +94 + 67 = 267

For using 15 , total = 234

Using the booleans that I suggested, you will end up with 200

\$\endgroup\$
  • \$\begingroup\$ I am not sure what this answer is trying to say. How are you counting the number of divisions? There are no divisions in the code. Maybe you mean “modulo operations” or “method invocations”? Currently, your code snippet is full of magic numbers without much indication what they could mean. Your analysis is probably interesting, but you need to explain what you did. Also, Ruby uses a hash/octothorpe # to introduce comments. \$\endgroup\$ – amon Dec 27 '14 at 16:23
  • \$\begingroup\$ @amon yes, it seems like thepace was trying to count the number of modulo operations. I also found the answer unclear, but I managed to "decipher" it and tried to improve it. \$\endgroup\$ – Simon Forsberg Dec 27 '14 at 16:29
  • 2
    \$\begingroup\$ By the way, it is perfectly possible to do a FizzBuzz with 0 modulo operations: codereview.stackexchange.com/a/56896/31562 . It would increase other operations though. And honestly, 267, 234, or 200 modulo's... is the minor possible performance gain really worth it? \$\endgroup\$ – Simon Forsberg Dec 27 '14 at 16:31

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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