Refactoring in Ruby

In my last post, I wrote a code-snippet that admittedly was not a very good example of well-refactored ruby. I'll let you go find it yourself, since I have no desire to link to something that embarassing ;), but I thought today I'd show that I learn from my mistakes by walking through some refactoring I did on my startup's app this morning.

First of all, what is refactoring? (Yes, somebody might be reading this who doesn't know). You can see this wikipedia entry for a complete description, but the short version is this: refactoring is changing the code without changing it's behavior. Ideally, after you refactor, the code does the exact same thing it did before. The difference is that it also:

(a) is easier to read
(b) contains less duplication
(c) is less complex, and
(d) is more flexible towards future changes.

So without any further ado, here is my starting code:

class SomeModel < ActiveRecord::Base
  has_many :objectives
  has_many :activities

  def stats_to_json
    json = "[["
    load_json_objects(self.objectives,json)  
    json << "],[" 
    load_json_objects(self.activities,json)    
    json << "]]"
    return json
  end
  
private
  def load_json_objects(collection,json)
    i = 1
    collection.each do |item|
      json << "{display:\"#{item.name}\",value:\"#{item.id}\"}"
      i += 1
      if i <= collection.length
        json << ","
      end
    end  
  end
end

This does what I want it to, it takes the objectives and activities collections from the ActiveRecord associations and transforms them into a simple JSON array with a "display" member and a "value" member for each object (this is useful for me when I'm generating tags dynamically). However, although it's functional, it's pretty ugly. Here's what I did to clean it up:

First of all, passing the "json" string back and forth is unnecessary. Interpolation is faster anyway, so I stopped sending that second string parameter back and forth and turned that whole public method into one interpolated string.

class SomeModel < ActiveRecord::Base
  has_many :objectives
  has_many :activities

  def stats_to_json
    "[[#{load_json_objects(self.objectives)}],[#{load_json_objects(self.activities)}]]"
  end
  
private
  def load_json_objects(collection)
    json = ""
    i = 1
    collection.each do |item|
      json << "{display:\"#{item.name}\",value:\"#{item.id}\"}"
      i += 1
      if i <= collection.length
        json << ","
      end
    end
    return json  
  end
end

That's a little better. Next, you can see that I'm using "i" as my own makeshift iterator. It works, but ruby provides a better method for this with the "each_with_index" method, which does a similar thing but a little cleaner.

class SomeModel < ActiveRecord::Base
  has_many :objectives
  has_many :activities

  def stats_to_json
    "[[#{load_json_objects(self.objectives)}],[#{load_json_objects(self.activities)}]]"
  end
  
private
  def load_json_objects(collection)
    json = ""
    collection.each_with_index do |item,i|
      json << "{display:\"#{item.name}\",value:\"#{item.id}\"}"
      if i <= collection.length - 1
        json << ","
      end
    end
    return json  
  end
end

That's a little better, but after some more thought I realized that the end result of this logic was simply not applying the last comma. That could be easily done by just removing the last character of the string with the "chop" method rather than doing that extra "if" block in each iteration. This slims it down even more, and it turns out I didn't need the index at all:

class SomeModel < ActiveRecord::Base
  has_many :objectives
  has_many :activities

  def stats_to_json
    "[[#{load_json_objects(self.objectives)}],[#{load_json_objects(self.activities)}]]"
  end
  
private
  def load_json_objects(collection)
    json = ""
    collection.each do |item|
      json << "{display:\"#{item.name}\",value:\"#{item.id}\"},"
    end
    return json.chop
  end
end

Now we're cooking. OK, what about the each block? conventionally, if you have only one line to execute in the block, you use brackets instead of "do...end", so I made that change next:

class SomeModel < ActiveRecord::Base
  has_many :objectives
  has_many :activities

  def stats_to_json
    "[[#{load_json_objects(self.objectives)}],[#{load_json_objects(self.activities)}]]"
  end
  
private
  def load_json_objects(collection)
    json = ""
    collection.each {|item| json << "{display:\"#{item.name}\",value:\"#{item.id}\"},"}
    return json.chop  
  end
end

The next thing I spotted that I didn't like was in that string in the public method. Look at all those square brackets! Do I really need them all right there? The answer, in my opinion, was no, since each collection was being bracketed the same. Thus, I moved the inner sets of square brackets into the private method, like so:

class SomeModel < ActiveRecord::Base
  has_many :objectives
  has_many :activities

  def stats_to_json
    "[#{load_json_objects(self.objectives)},#{load_json_objects(self.activities)}]"
  end
  
private
  def load_json_objects(collection)
    json = ""
    collection.each {|item| json << "{display:\"#{item.name}\",value:\"#{item.id}\"},"}
    "[#{json.chop}]" 
  end
end

After slimming it down this far, I felt there wasn't much else to do to shorten it. However, there are a few ways to make it more reusable. First, the private method assumes every collection passed into it will always be composed of elements that have a "name" method, and an "id" method. Since there is no such guarantee, and I might want to use it on other collections later, I added some default arguments to the "load_json_objects" method:

class SomeModel < ActiveRecord::Base
  has_many :objectives
  has_many :activities

  def stats_to_json
    "[#{load_json_objects(self.objectives)},#{load_json_objects(self.activities)}]"
  end
  
private
  def load_json_objects(collection, display_method = :name, value_method = :id)
    json = ""
    collection.each {|item| json << "{display:\"#{item.send(method)}\",value:\"#{item.send(value_method)}\"},"}
    "[#{json.chop}]" 
  end
end

Finally, I decided that I might want this functionality in more than one model class (in fact, I needed it in another class immediately) so I decided to push it out into a module that can be mixed in at any time.

module JsonExtensions
  
  module CollectionTransformer    
    def c_to_j(collection, display_method=:name, value_method=:id)
      json = ""
      collection.each do |item| 
        json << "{display:\"#{item.send(display_method)}\","
        json << "value:\"#{item.send(value_method)}\"}," 
      end
      "[#{json.chop}]"
    end    
  end
end

class SomeModel < ActiveRecord::Base

  include JsonExtensions::CollectionTransformer

  has_many :objectives
  has_many :activities

  def stats_to_json
    "[#{c_to_j(self.objectives)},#{c_to_j(self.activities)}]"
  end
end

There you have it, a solid refactor. Everything behaves the way it did before (with tests passing to prove it), but it's easier to read, contains less duplication, and is more reusable. Rinse and repeat!

EDIT:You'll have to take my word for it when I say it's easier to read, since it's pretty tough to read any non-trivial code in this blog-format. ;)