26
\$\begingroup\$

I'm super new to this so please be nice. I tried solving Chris Pine's challenge to print the entire lyrics to 99 bottles of beer on the wall. This seems to work, but is there a better/more efficient way to do it? Just want to learn how to improve it if I can.

var1 = 99
loop do
    puts (var1.to_s) + ' bottles of beer on the wall, ' + (var1.to_s) + ' bottles of beer, ' 
    puts 'take one down, pass it around, ' + ((var1 - 1).to_s) + ' bottles of beer on the wall.' 
    var1 = (var1 - 1)
break if var1 == 1
end
\$\endgroup\$
5
  • 7
    \$\begingroup\$ Welcome to Code Review! Glad that you found this site at this early stage of learning programming, I'm sure you will gain valuable insight by reviewers here! \$\endgroup\$
    – Phrancis
    Feb 1, 2015 at 4:43
  • 3
    \$\begingroup\$ May I suggest to change the text into: 99 little bugs in the code 99 little bugs in the code Take one down, patch it around 117 little bugs in the code ;) \$\endgroup\$
    – DJanssens
    Feb 1, 2015 at 14:54
  • 1
    \$\begingroup\$ "127 little bugs in the code, 127 little bugs. Feature request adds one more, -127 little bugs in the code" \$\endgroup\$
    – Flambino
    Feb 1, 2015 at 22:41
  • \$\begingroup\$ Just one? @Flambino must be good, real good. \$\endgroup\$ Feb 4, 2015 at 21:18
  • \$\begingroup\$ @CarySwoveland Well, with the unintended rollover it's at least 2 bugs. But maybe the bug count is a plain 32 bit int, in which case the feature request actually added 4,294,967,042 new bugs. The truth is probably somewhere between those two extremes. \$\endgroup\$
    – Flambino
    Feb 4, 2015 at 21:26

2 Answers 2

29
\$\begingroup\$

In any programming language you should always use meaningful names for your variables, functions, methods, and whatever else you need to assign a name.

In your case, var1 is completely non-descriptive. A better name would be bottles, bottle_count or bottles_remaining to be even more specific.

Secondly, in Ruby, you rarely need to use a plain loop. In your case, you basically want to repeat roughly the same thing 99 times (well actually, no, you don't exactly, but we'll get to that later).

A simple way to do this in Ruby is to say:

99.downto(1) do |remaining|
  # stuff to do 99 times
end

The remaining variable will start at 99 and count down to 1, so we can use it to print the verses.

And speaking of printing, you're making things difficult for yourself by not using string interpolation.

If you use double quotes instead of single quotes, you can use #{...} to insert stuff into strings. Like so:

99.downto(1) do |remaining|
  puts "#{remaining} bottles of beer on the wall, #{remaining} bottles of beer."
  puts "Take one down and pass it around, #{remaining-1} bottles of beer on the wall."
  puts # print a blank line between verses.
end

Note that #{...} can contain any Ruby code (but keep it simple for your own sake!), so a bit of arithmetic to subtract 1 works too. And there's no need to use to_s everywhere, as that happens automatically when using string interpolation.

The above code is pretty much equivalent to your current code, except that it prints a blank line between verses. And the fact that it prints 99 verses - you're actually only printing 98 verses in your version.

... However, it's printing wrong lyrics. Grammatically incorrect lyrics, at any rate.

Most verses do indeed go like this:

X bottles of beer on the wall, X bottles of beer.
Take one down and pass it around, (X-1) bottles of beer on the wall.

where X is whatever number of bottles we have left. (Note, by the way, how much this looks like the Ruby code above; the code's almost as simple to read.)

But notice the changes in the last 3 verses:

2 bottles of beer on the wall, 2 bottles of beer.
Take one down and pass it around, 1 bottle of beer on the wall.

1 bottle of beer on the wall, 1 bottle of beer.
Take it down and pass it around, no more bottles of beer on the wall.

No more bottles of beer on the wall, no more bottles of beer.
Go to the store and buy some more, 99 bottles of beer on the wall.

And that's why it's a programming challenge. If it was just about writing those 5 lines of code above, it wouldn't be too tricky.

But what's interesting is when you have exceptions to the rules, and those last 3 verses are just that. They don't even follow a single, different set of rules - they're each different in their own way.

How you go about handling that is up to you, though. It's a challenge, after all. There are clever ways, simple ways, complex ways, nonsensical ways... which one you pick is up to you. The idea is to make you consider the problem and its potential solutions; there isn't a single correct answer. The destination is already given (print the lyrics), so it's all about the journey.


Other small stuff:

  • The Ruby convention is to use 2 spaces of indentation - not 4 spaces, not tabs.
  • The break line should be indented.
  • The parentheses in var1 = (var1 - 1) are completely unnecessary. Furthermore, you can write it as var1 -= 1, which is a shorthand for the same thing.
\$\endgroup\$
5
  • 2
    \$\begingroup\$ I've seen (and written) production code where special cases somewhat like the last three verses were implemented: singular vs. plural numbers, and sometimes alternative phrases. But one has to consider the complexity of different ways of encoding these special cases into large chunks of each iteration; even if this is code that is expected to last a long time and need to be maintained, I'd likely put just the first 97 verses in the loop and print the last three verses literally line by line at the end. \$\endgroup\$
    – David K
    Feb 1, 2015 at 13:11
  • \$\begingroup\$ @DavidK Sure, that's a valid solution. The real point of the challenge isn't so much to find "the only right solution™", as much as it is to get the programmer to think a little and consider what solutions even exist. Hardcoding the last 3 verses is indeed a solution - a nice and simple one, too. Unless there are requirements for one thing or the other, the implementation is completely up to the programmer. \$\endgroup\$
    – Flambino
    Feb 1, 2015 at 14:07
  • 1
    \$\begingroup\$ I agree. You raised an important point, which is that people often neglect the cases that slightly alter the pattern seen in most cases. You will sometimes see production code that does the equivalent of printing "1 bottles". As you point out, there are many ways to resolve this; I'm simply casting a vote for the "simple" solution in this particular case. \$\endgroup\$
    – David K
    Feb 1, 2015 at 15:19
  • 1
    \$\begingroup\$ @DavidK Oh, man, that's a pet peeve of mine. I cringe slightly when I see stuff like "1 new mail(s)" instead of proper pluralization. I admit I'm guilty of doing it myself, but it always make me think someone (like me) just didn't care. Doesn't inspire confidence in the software. \$\endgroup\$
    – Flambino
    Feb 1, 2015 at 16:20
  • \$\begingroup\$ a better name would be n \$\endgroup\$ Feb 3, 2015 at 14:09
5
\$\begingroup\$

That looks pretty good. You might consider using an enumerator and block to avoid the need to write:

var = (var1 - 1)

(which one would normally write var -= 1) and to break out of the loop. For example, use Integer#downto:

99.downto(1) do |n|
  puts "#{n} bottles of beer on the wall, #{n} bottles of beer, " 
  puts "take one down, pass it around, #{n-1} bottles of beer on the wall." 
end

Alternatively, you could create an array to be enumerated with Array#each:

nbr = 99.downto(0).to_a
n = nbr.shift
while nbr.any?
  puts "#{n} bottles of beer on the wall, #{n} bottles of beer, " 
  n = nbr.shift
  puts "take one down, pass it around, #{n} bottles of beer on the wall." 
end

A third way would be to use an enumerator directly and step through it with Enumerator#next:

enum = Enumerator.new { |y| 100.times { |i| y << 99-i } }
n = enum.next
loop do
   puts "#{n} bottles of beer on the wall, #{n} bottles of beer, " 
   n = enum.next
   puts "take one down, pass it around, #{n} bottles of beer on the wall." 
end

next raises a StopIteration exception when an attempt is made to go beyond the end of the iterator. Kernel handles the exception by breaking out of the loop.

\$\endgroup\$
0

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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