Can anyone offer any criticism on the following at a simple FizzBuzz in terms of Ruby idioms or otherwise?
class FizzBuzz
def initialize(fizz = 'Fizz', buzz = 'Buzz')
@fizz = fizz
@buzz = buzz
end
def is_number_divisible_by?(number, divisor)
number % divisor == 0
end
def get_text(number)
fizz_buzz_text = ''
if is_number_divisible_by?(number, 3)
fizz_buzz_text += @fizz
end
if is_number_divisible_by?(number, 5)
fizz_buzz_text += @buzz
end
return fizz_buzz_text
end
end
fb = FizzBuzz.new
(1...101).each do |i|
fb_text = fb.get_text(i)
puts fb_text.empty? ? i : fb_text
end
2 Answers 2
First off: If you want a range that goes from 1 to x, inclusive, write (1..x)
, not (1...x+1)
.
Second, if it's the last line executed in a method, it's automatically returned -- so in get_text
, you don't need return
in the last line, just the actual code.
Also, get_text
should return the text to be printed, not what it might be. Move the empty check (fb_text.empty ? i : fb_text
) to get_text
instead of having it at the puts
call. See the code at the end for clarification.
if
statements with just one line should be expressions at the end. Again, see the modified code for what I mean.
Methods defined in a class are generally talking about the object they're run on. I'd pull is_number_divisible_by?
out of the class and dropping it at the beginning of your code instead, and renaming it to something like divisible_by?
instead (since it's shorter and carries the same information).
Aside from that (and an odd extra newline at the beginning of your class definition), it looks good! Here's your code with these suggestions:
def divisible_by?(number, divisor)
number % divisor == 0
end
class FizzBuzz
def initialize(fizz = 'Fizz', buzz = 'Buzz')
@fizz = fizz
@buzz = buzz
end
def get_text(number)
fizz_buzz_text = ''
fizz_buzz_text += @fizz if divisible_by? number, 3
fizz_buzz_text += @buzz if divisible_by? number, 5
fizz_buzz_text.empty? ? number : fizz_buzz_text
end
end
fb = FizzBuzz.new
(1..100).each do |i|
puts fb.get_text i
end
Note: Personally, I wouldn't do this in an OOP way. Ruby is much more oriented towards functional programming, though of course OOP is perfectly valid; besides, functional is a bit simpler. My preferred way to do it would be something like this:
(1..100).each do |num|
output = ''
output += 'Fizz' if num % 3 == 0
output += 'Buzz' if num % 5 == 0
puts output.empty? ? num : output
end
QPaysTaxes has a lot of great points, but one thing that stands out is that you are asking for the "Idiomatic" way of doing things in ruby. For fizzbuzz, that would involve a hash mapping numbers to words so you could easily and arbitrarily extend it. I would rewrite QPaysTaxes get_text
function as such to get more idiomatic ruby:
# in class FizzBuzz
TEXT = {
3 => "Fizz",
5 => "Buzz",
# 7 => "Jazz",
# 11 => "Guzz",
} # or set this dynamically during initialization, whatever
def get_text(number)
fizz_buzz_text = TEXT.inject("") do |str, rule|
number % rule[0] == 0 ? str + rule[1] : str
end
fizz_buzz_text.empty? ? number : fizz_buzz_text
end
Now you can uncomment those extra rules or throw your own in with ease. If the method for determining replacement or not changes from being num divisible by rule
to ((num * 6) + 4) is divisible by rule
, you only need to change a single location instead of once per rule. (This practice is called DRY, or "Don't Repeat Yourself".)
I can explain the code further if you want, just leave a comment below.
-
\$\begingroup\$ I'm almost certain that you should be using
''
for strings wherever possible, like ininject("")
. Could be wrong on that, though. \$\endgroup\$anon– anon2015年06月08日 22:44:25 +00:00Commented Jun 8, 2015 at 22:44 -
2\$\begingroup\$ @QPaysTaxes Style guide says you can use either, as long as you remain consistent (which I did not, I fixed it). \$\endgroup\$Devon Parsons– Devon Parsons2015年06月08日 23:59:35 +00:00Commented Jun 8, 2015 at 23:59
-
1\$\begingroup\$ I dont think this makes the code more idiomatic? What you have done is tried to make it more extensible and also isnt it breaking the YAGNI principle? fizz_buzz becomes less relevant here for names now also - since you have tried to make it more general purpose anyway? \$\endgroup\$user1189880– user11898802015年06月09日 15:42:43 +00:00Commented Jun 9, 2015 at 15:42
-
2\$\begingroup\$ Fizz buzz is the general name of this problem, it doesn't matter what the specific text is. And whether or not you'll need it is on a case by case basis. Obviously for just FizzBuzz you do not, but the purpose of learning FizzBuzz is so that you can apply it to real world problems, where you are far more likely to need to keep things extensible. \$\endgroup\$Devon Parsons– Devon Parsons2015年06月09日 16:33:29 +00:00Commented Jun 9, 2015 at 16:33