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?
-
\$\begingroup\$ 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). \$\endgroup\$Rainbolt– Rainbolt2014年07月14日 18:40:57 +00:00Commented Jul 14, 2014 at 18:40
5 Answers 5
- 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
anddivisibleBy5
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}
-
\$\begingroup\$ The program was originally built to be code golf, and thus the odd variable names. But I've still learnt something today, thanks! \$\endgroup\$user25840– user258402014年07月13日 16:05:47 +00:00Commented Jul 13, 2014 at 16:05
-
3\$\begingroup\$ A PascalCased method name? My Ruby-sense is tingling... \$\endgroup\$Flambino– Flambino2014年07月13日 16:50:22 +00:00Commented Jul 13, 2014 at 16:50
-
3\$\begingroup\$ It would be fun to extend
Integer
to hold thedivisibleBy3?
anddivisbleBy5?
methods. \$\endgroup\$Schism– Schism2014年07月13日 18:07:47 +00:00Commented Jul 13, 2014 at 18:07 -
1\$\begingroup\$ Ah, yup, there we go, the very instant I posted my last comment :) \$\endgroup\$Flambino– Flambino2014年07月13日 19:03:01 +00:00Commented Jul 13, 2014 at 19:03
-
4\$\begingroup\$ I'd move the
puts
to theeach
block. More modular, less repetition. \$\endgroup\$tokland– tokland2014年07月15日 08:40:28 +00:00Commented Jul 15, 2014 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
-
\$\begingroup\$ @Vogel612 I was unaware that that was a requirement. Where do you see that? \$\endgroup\$Flambino– Flambino2014年07月13日 23:22:02 +00:00Commented Jul 13, 2014 at 23:22
-
\$\begingroup\$ 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 .. \$\endgroup\$Vogel612– Vogel6122014年07月13日 23:25:35 +00:00Commented Jul 13, 2014 at 23:25
-
\$\begingroup\$ @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. \$\endgroup\$Flambino– Flambino2014年07月13日 23:39:50 +00:00Commented Jul 13, 2014 at 23:39
-
3\$\begingroup\$ @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. \$\endgroup\$Flambino– Flambino2014年07月14日 01:27:25 +00:00Commented Jul 14, 2014 at 1:27
-
3\$\begingroup\$ 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. \$\endgroup\$Blorgbeard– Blorgbeard2014年07月14日 03:35:16 +00:00Commented Jul 14, 2014 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 yield
ing 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).
-
1\$\begingroup\$ @Vogel612 Not sure what your objection is, or whether you're being serious. \$\endgroup\$200_success– 200_success2014年07月13日 23:44:57 +00:00Commented Jul 13, 2014 at 23:44
-
1\$\begingroup\$ 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. \$\endgroup\$Vogel612– Vogel6122014年07月14日 08:55:35 +00:00Commented Jul 14, 2014 at 8:55
-
4\$\begingroup\$ @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. \$\endgroup\$200_success– 200_success2014年07月14日 10:10:34 +00:00Commented Jul 14, 2014 at 10:10
-
1\$\begingroup\$ @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? \$\endgroup\$cHao– cHao2014年07月14日 13:06:18 +00:00Commented Jul 14, 2014 at 13:06
-
1\$\begingroup\$ @Vogel612: Except that the acceptance criteria are met.
fizz = 'Fizz'; buzz = 'Buzz'; puts ("#{fizz}#{buzz}" == "FizzBuzz")
outputs "true". \$\endgroup\$cHao– cHao2014年07月14日 14:22:02 +00:00Commented Jul 14, 2014 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).
-
1\$\begingroup\$ +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 simpleeach
, though \$\endgroup\$Flambino– Flambino2014年07月14日 11:55:15 +00:00Commented Jul 14, 2014 at 11:55 -
1\$\begingroup\$ @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, thenprint_lines(lines)
or whatever. \$\endgroup\$tokland– tokland2014年07月14日 11:57:07 +00:00Commented Jul 14, 2014 at 11:57 -
\$\begingroup\$
self
is required in the definition ofdivisible_by
. An even simpler example isclass Integer; def mod(n); % n; end; end
.4.mod(3) => "n\n"
. Do you know the rule that requiresself
to be explicit here? (Other rules, for example, are that write accessors must includeself.
andself.class
is required to obtainself
's class.) \$\endgroup\$Cary Swoveland– Cary Swoveland2014年12月27日 20:22:07 +00:00Commented Dec 27, 2014 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.
-
\$\begingroup\$ 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. =;)- \$\endgroup\$RubberDuck– RubberDuck2014年12月27日 03:03:44 +00:00Commented Dec 27, 2014 at 3:03 -
1\$\begingroup\$ 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. \$\endgroup\$DGM– DGM2014年12月27日 05:24:51 +00:00Commented Dec 27, 2014 at 5:24