Rubies in the Rough

Objectified Beer

I got another thing out of my recent conversation with Katrina Owen: a will-not-let-go itch to try a programming exercise that she mentioned. I'm such a sucker for a challenge.

Katrina spoke of an assignment that her and Sandi Metz have used in their object orientation trainings. She said that they build an OO implementation of the 99 Bottles of Beer song, "mainly removing ifs and such." There may be more to the actual task than this, but I ran with that brief explanation.

As I often say, you'll probably learn more by trying the exercise for yourself before you read through my thoughts about it. Give it a go if you can spare the time.

Diff Driven Development

I decided to throw some scaffolding into the master branch of a Git repository. I figured I could then branch off of that with each idea, to keep trying things out.

I started by constructing a trivial framework for running and verifying the song. That consisted of an executable:

#!/usr/bin/env ruby -w

require_relative "../lib/bottles_of_beer"

verses = ARGV.first =~ /\A\d+\z/ ? ARGV.shift.to_i : 99
BottlesOfBeer::Song.new(verses).sing($stdout)

a copy and paste implementation:

# -*- coding: utf-8 -*-

module BottlesOfBeer
  class Song
    def initialize(verses = 99)
      @verses = verses
    end

    attr_reader :verses
    private     :verses

    def sing(target = $stdout)
      target << <<END_SONG
99 bottles of beer on the wall, 99 bottles of beer.
Take one down and pass it around, 98 bottles of beer on the wall…

…
END_SONG
    end
  end
end

and a Rake task to check the output against a static file I copied from the Web as my definitive source of the lyrics:

desc "Verifies the song output."
task :spec do
  print "\e[31m"
  system "bash -c 'diff -u <(bin/bottles_of_beer) spec/full_song.txt'"
  print "\e[0m"
  if $?.success?
    puts "\e[32mPerfect match.\e[0m"
  end
end

task default: :spec

This system of validation was very important to me. I wanted it to be easy to see how close to correct I was and a diff sums that up beautifully. However, I did not want to test drive my playing around. That would add overhead to my experimentation that I just don't need for a problem that I can keep in my head like this.

A Swing and a Miss

With the setup done, I was ready to try a solution.

My first idea was pretty straightforward. Build up the needed verses and then write them out:

require_relative "verse"
require_relative "penultimate_verse"
require_relative "ultimate_verse"

module BottlesOfBeer
  class Song
    def initialize(verses = 99)
      @verses  = Array.new(verses - 1) { |i| Verse.new(verses - i) }
      @verses << PenultimateVerse.new
      @verses << UltimateVerse.new(verses)
    end

    attr_reader :verses
    private     :verses

    def sing(target = $stdout)
      verses.each do |verse|
        target << verse
      end
    end
  end
end

Most verses are represented by a fairly predicable object:

# -*- coding: utf-8 -*-

module BottlesOfBeer
  class Verse
    def initialize(bottles)
      @bottles = bottles
    end

    attr_reader :bottles
    private     :bottles

    def to_s
      "#{bottles} bottles of beer on the wall, " +
      "#{bottles} bottles of beer.\n"            +
      "Take one down and pass it around, "       +
      "#{bottles - 1} bottles of beer on the wall…\n\n"
    end
  end
end

However, the one bottle verse is an edge case:

require_relative "verse"

module BottlesOfBeer
  class PenultimateVerse < Verse
    def initialize
      super(1)
    end

    def to_s
      super.gsub("1 bottles", "1 bottle")
    end
  end
end

The last verse is also special:

# -*- coding: utf-8 -*-

require_relative "verse"

module BottlesOfBeer
  class UltimateVerse < Verse
    def to_s
      "No more bottles of beer on the wall, " +
      "no more bottles of beer.\n"            +
      "Go to the store and buy some more, "   +
      "#{bottles} bottles of beer on the wall…\n"
    end
  end
end

This solution gets close, but it doesn't quite work:

--- /dev/fd/63  2014-05-30 13:21:03.000000000 -0500
+++ spec/full_song.txt  2014-05-30 09:34:28.000000000 -0500
@@ -290,10 +290,10 @@
 Take one down and pass it around, 2 bottles of beer on the wall…

 2 bottles of beer on the wall, 2 bottles of beer.
-Take one down and pass it around, 1 bottles of beer on the wall…
+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 one down and pass it around, 0 bottles of beer on the wall…
+Take one 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…

This was an interesting decision point for me in the exercise. Obviously, I'm not green. Eventually, I need to get there. But I'm really close. Only two lines are wrong and I can see what the problem is. Mainly, when I transition to new verses, I don't pick up on the edge cases. I was starting to have ideas about how I might address this issue, but I wasn't too happy with the rest of the code yet anyway. I decided to attempt to improve what I had built a little first, before I tackled the obvious bug. I feel like my diff-based validation gave me the confidence that I could do this, since I would notice by the line count alone if I was drifting further off track.

Can I get a side of objects with that?

OK, my first major complaint was that I didn't land on a very OO solution. Since that's the point of the exercise, I wanted to address it. More specifically I think overriding a method to call gsub() on its output largely misses to point of sharing an implementation, as objects do. Similarly, my UltimateVerse pretty much ignores everything from its superclass. I minted a new branch and broke out the polish.

I began by extracting a few methods in Verse so I could override them in subclasses:

# -*- coding: utf-8 -*-

module BottlesOfBeer
  class Verse
    def initialize(bottles)
      @bottles = bottles
    end

    attr_reader :bottles
    private     :bottles

    def to_s
      "#{count_with_units} of beer on the wall, " +
      "#{count_with_units.downcase} of beer.\n"   +
      "#{action}, "                               +
      "#{bottles - 1} bottles of beer on the wall…\n\n"
    end

    private

    def count_with_units
      "#{bottles} bottles"
    end

    def action
      "Take one down and pass it around"
    end
  end
end

That allowed me to clean up the content editing in the one bottle verse:

require_relative "verse"

module BottlesOfBeer
  class PenultimateVerse < Verse
    def initialize
      super(1)
    end

    def count_with_units
      "1 bottle"
    end
  end
end

It also allowed me to turn the no more bottles verse into a more proper subclass that doesn't just override the work of the superclass:

require_relative "verse"

module BottlesOfBeer
  class UltimateVerse < Verse
    def to_s
      super.strip + "\n"
    end

    private

    def count_with_units
      "No more bottles"
    end

    def action
      "Go to the store and buy some more"
    end
  end
end

While things are definitely more object oriented in this version, I've actually worsened the results by one more failing line:

--- /dev/fd/63  2014-05-30 13:36:26.000000000 -0500
+++ spec/full_song.txt  2014-05-30 09:34:28.000000000 -0500
@@ -290,10 +290,10 @@
 Take one down and pass it around, 2 bottles of beer on the wall…

 2 bottles of beer on the wall, 2 bottles of beer.
-Take one down and pass it around, 1 bottles of beer on the wall…
+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 one down and pass it around, 0 bottles of beer on the wall…
+Take one 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, 98 bottles of beer on the wall…
+Go to the store and buy some more, 99 bottles of beer on the wall…

I'm still quite close to correct, but I am seeing that drift I decided to watch for. This told me it was time to get serious about reaching a real solution.

Go Green or Go Home

My problems come from the fact that the verses transition into the next part of the song. That leads me to another thing that bothered me with my initial code: the need for bottles - 1 in Verse. If Song is keeping track of the current Verse, it feels like they shouldn't need to peek ahead internally. This made me feel like a layer was missing and I set out to add it in a new branch.

I decided to introduce this new concept of verses transitioning to the next verse:

# -*- coding: utf-8 -*-

module BottlesOfBeer
  class VerseWithTransition
    def initialize(verse, next_verse)
      @verse      = verse
      @next_verse = next_verse
    end

    attr_reader :verse, :next_verse
    private     :verse, :next_verse

    def to_s
      "#{verse}, #{next_verse.opening.downcase}…\n"
    end
  end
end

Of course, that required some changes in the Song to make use of these new transitions:

require_relative "verse"
require_relative "penultimate_verse"
require_relative "ultimate_verse"
require_relative "verse_with_transition"

module BottlesOfBeer
  class Song
    def initialize(verses = 99)
      @verses  = Array.new(verses - 1) { |i| Verse.new(verses - i) }
      @verses << PenultimateVerse.new
      @verses << UltimateVerse.new(verses)
    end

    attr_reader :verses
    private     :verses

    def sing(target = $stdout)
      verses.each_cons(2) do |verse, next_verse|
        target << VerseWithTransition.new(verse, next_verse) << "\n"
      end
      target << VerseWithTransition.new(verses.last, verses.first)
    end
  end
end

The base Verse itself could now quit worrying about the next number, as long as it made its opening() available for reuse:

module BottlesOfBeer
  class Verse
    def initialize(bottles)
      @bottles = bottles
    end

    attr_reader :bottles
    private     :bottles

    def opening
      "#{count_with_units} of beer on the wall"
    end

    def to_s
      "#{opening}, "                            +
      "#{count_with_units.downcase} of beer.\n" +
      action
    end

    private

    def count_with_units
      "#{bottles} bottles"
    end

    def action
      "Take one down and pass it around"
    end
  end
end

The edge cases were largely unchanged in this round, save that I pulled out an override from UltimateVerse that twiddled whitespace. It felt more natural to handle that at the Song level. The low number of changes needed here made me feel like I was making better use of subclasses now.

Also, good news, my output was finally validating.

Things That Bug Me

Now that I was green, I wanted to attack other concerns I had with the code. My biggest remaining complaint was this line in Song:

      @verses  = Array.new(verses - 1) { |i| Verse.new(verses - i) }

This represents multiple problems, in my opinion. First, I don't like that I'm pregenerating all of the verses. What if we want to use a huge number of verses? Why should that be memory inefficient? I would prefer to do the work as it comes in, when possible.

Also, there's some very non-obvious index math in this code, if you ask me. I had to lower the size of the initial Array to make room for the edge cases and I had to reverse the indexes inside the block, because the lyrics count down.

Another branch led to another round of fixes.

Here's a more clean iteration through the verses, in my opinion:

require_relative "verse"
require_relative "penultimate_verse"
require_relative "ultimate_verse"
require_relative "verse_with_transition"

module BottlesOfBeer
  class Song
    def initialize(verses = 99)
      fail ArgumentError, "Cannot sing negative verses" if verses < 0

      @verses = verses
    end

    attr_reader :verses
    private     :verses

    def sing(target = $stdout)
      verses.downto(2).each_cons(2) do |bottles, next_bottles|
        write_verse(target, Verse.new(bottles), Verse.new(next_bottles))
      end
      write_verse(target, Verse.new(2),         PenultimateVerse.new) \
        if verses >= 2
      write_verse(target, PenultimateVerse.new, UltimateVerse.new) \
        if verses >= 1
      write_verse(target, UltimateVerse.new,    Verse.new(verses), "")
    end

    private

    def write_verse(target, verse, next_verse, padding = "\n")
      target << VerseWithTransition.new(verse, next_verse) << padding
    end
  end
end

The main difference here is that I feel the iterators downto() and each_cons() make it easier to see how I am moving through the data. There's a lot more object construction in this version, but that's a concern for the garbage collector (which has improved a ton in recent releases). I try not to be afraid of using lots of objects, until I have some reason to be.

Just for the record, there was one more tiny change that I didn't show here. I finally dropped the argument to UltimateVerse's constructor, since that's now handled by transitions. This required that I override the base constructor to ignore it.

The Big Questions

Now that I had a working solution, I found myself wondering how much extra work I had done to objectify this code that could have a pretty short solution. I pulled a fresh branch off of master and solved the problem again for comparison purposes.

I rewrote the Song using the least complex proceedural code that I could dream up:

# -*- coding: utf-8 -*-

module BottlesOfBeer
  class Song
    def initialize(verses = 99)
      fail ArgumentError, "Cannot sing negative verses" if verses < 0

      @verses = verses
    end

    attr_reader :verses
    private     :verses

    def sing(target = $stdout)
      verses.downto(0) do |count|
        action  = count.zero? ? "Go to the store and buy some more"
                              : "Take one down and pass it around"
        target << "#{bottles(count)} of beer on the wall, " +
                  "#{bottles(count).downcase} of beer.\n"   +
                  "#{action}, "                             +
                  "#{bottles(count - 1).downcase} of beer on the wall…\n"
        target << "\n" unless count.zero?
      end
    end

    private

    def bottles(count)
      if count < 0
        bottles(verses)
      elsif count.zero?
        "No more bottles"
      else
        "#{count} bottle#{'s' unless count == 1}"
      end
    end
  end
end

I'll be honest and admit that I expected to like this code better. This is a small problem and I can keep the whole thing in my head when working at this level. I figured a short chunk of code that reflected that would be pretty digestible.

After seeing the two solutions side-by-side though, I'm not sure I really do feel that way. Sure, this one is shorter and you can easily view the whole thing at once, but there's a ton of edge cases mashed into this small space that make it harder to reason about. At least that's what I think. Notice the three separate count.zero? checks and the return of count - 1 even though the iterator is pushing us through the verses. I'm pretty sure I would rather fix bugs in the objectified code, rather than reason out which rules are in effect here at any given time.

Thanks for the mental workout Katrina.