Forums » ruby » Refactoring (#75)

The three rules of Ruby Quiz:

1.  Please do not post any solutions or spoiler discussion for this quiz 
until
48 hours have passed from the time on this message.

2.  Support Ruby Quiz by submitting ideas as often as you can:

http://www.rubyquiz.com/

3.  Enjoy!

Suggestion:  A [QUIZ] in the subject of emails about the problem helps 
everyone
on Ruby Talk follow the discussion.

-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=

by Pat Eyler

This week's quiz is a bit of a departure from the normal.  Instead of 
submitting
different implementations of the same code, we'd like you to submit 
different
implementations of the same process -- Refactoring.

Refactoring is the art of improving the design of existing code, without
changing it's functional behaviour.  It is well documented in the book
'Refactoring' by Martin Fowler, and on his website:

	http://www.refactoring.com

The quiz this week is to submit refactorings of code you use -- whether 
your own
code, a library or application for RubyForge, or even something from the
Standard Library.  Any submission should implement a refactoring from:

	http://www.refactoring.com/catalog/index.html

or other citable sources, e.g.:

	http://kallokain.blogspot.com/2006/01/refactoring-extract-mixin.html

Each Submission should follow this outline:

	Refactoring name (and citation if needed)
	Original code
	Explanation of the purpose and mechanics of the refactoring
	New code
	(optionally, unit tests created/used to verify the code)

Submissions will be combined into an online catalog of Ruby 
Refactorings,
probably on the RubyGarden wiki.

On 4/14/06, Ruby Quiz <james@grayproductions.net> wrote:
> Submissions will be combined into an online catalog of Ruby Refactorings,
> probably on the RubyGarden wiki.
>

I've created a page on the wiki to hold refactorings as they are 
submitted.

  http://www.rubygarden.org/ruby?RubyRefactoringCatalog

On Apr 14, 2006, at 7:46 AM, Ruby Quiz wrote:

> by Pat Eyler
>
> This week's quiz is a bit of a departure from the normal.  Instead  
> of submitting
> different implementations of the same code, we'd like you to submit  
> different
> implementations of the same process -- Refactoring.

I'll start us off this week.  (Thanks for the easy but educational
quiz Pat!)

James Edward Gray II

Replace Conditional with Polymorphism
=====================================

This is a traditional refactoring pattern located on page 255 of my
copy of the book this quiz is named after.  It can also be found on
the [Refactoring Catalog site](http://www.refactoring.com/catalog/
replaceConditionalWithPolymorphism.html).

Smelly Code
===========

This is actual code from a Rails application we are building at
work.  I noticed the problematic method when bug hunting in the
system the same day I had been reading in Refactoring.  (This method
is not related to the bug I was after, it works fine.)

Here are the relevant pieces of the three classes involved:

     class TimePeriod < ActiveRecord::Base
       #------------------
       # VALIDATIONS
       #------------------
       validates_presence_of :name, :start_date, :end_date

       #------------------
       # INSTANCE METHODS
       #------------------
       #
       # Returns +true+ if the passed +date+ is in the TimePeriod,
but not in any
       # attached closed day ranges.
       #
       def include?( date )
         case self
         when ClosedPeriod
           (start_date..end_date).include?(date)
         else
           (start_date..end_date).include?(date) and
           not closed_periods.any? { |closed| closed.include?(date) }
         end
       end
     end

     class SchedulePeriod < TimePeriod
       #------------------
       # ASSOCIATIONS
       #------------------
       has_many :closed_periods, :dependent => :destroy
     end

     class ClosedPeriod < TimePeriod

     end

These classes are used to represent simple time spans in our
application.  A SchedulePeriod might represent a semester at a
university, for example, which could have a related ClosedPeriod for
Spring Break.  (These are stored in the database in the time_periods
table, making use of ActiveRecord's Single Table Inheritance.)

What's Wrong Here and Why "Fix" It?
===================================

Obviously, the method I intend to refactor is the only one shown
TimePeriod#include?.  This method displays the code smell of a case
statement used to determine action based on object types.  Message
dispatch like this is Ruby's domain and not suited for case
statements.  I will use Replace Conditional with Polymorphism to
eliminate this.

It might be interesting to note that this method got to the current
state through a very natural process of code decay.  When the
database was first being established, we threw together a quick
TimePeriod class that looked something like:

     class TimePeriod < ActiveRecord::Base
       #------------------
       # ASSOCIATIONS
       #------------------
       has_many :closed_periods, :class_name  => "TimePeriod",
                                 :foriegn_key => "closed_period_id",
                                 :dependent   => :destroy

       #------------------
       # VALIDATIONS
       #------------------
       validates_presence_of :name, :start_date, :end_date

       #------------------
       # INSTANCE METHODS
       #------------------
       #
       # Returns +true+ if the passed +date+ is in the TimePeriod,
but not in any
       # attached closed day ranges.
       #
       def include?( date )
         (start_date..end_date).include?(date) and
         not closed_periods.any? { |closed| closed.include?(date) }
       end
     end

This is the same thing, but tracked using only one object that can
have "closed_periods" of its own type.  This worked just like the
current version does.  However, when we began working through the
controllers to create, find, and relate these objects, it became
apparent that it would be easier on us programmers to keep everything
straight if we broke them down into separate objects.  Thus the
change and the introduction of the smelly method.

Refactored Code
===============

Looking at the refactoring check list, I see that I first need to
cover the method with tests to ensure that I won't break anything.
Luckily, these were already in place for the code.  Here is one such
test, just to give you an idea of how they look:

     spring_break = TimePeriod.find_by_name("Spring Break")
     current = Date.civil(2006, 3, 1)
     loop do
       # all days in March should validate, except for Spring Break
       assert_equal( !spring_break.include?(current),
                     time_periods(:march).include?(current) )

       current += 1
       break if current.month == 4
     end

Now according to the refactoring catalog, I need to do two things.
The first is:  "Move each leg of the conditional to an overriding
method in a subclass."  That doesn't sound too tough.  Here's the
first such move:

     class ClosedPeriod < TimePeriod
       #------------------
       # INSTANCE METHODS
       #------------------
       # Returns +true+ if the passed +date+ is in the ClosedPeriod.
       def include?( date )
         (start_date..end_date).include?(date)
       end
     end

I ran the tests and nothing broke.  I'm now ready to move the second
leg:

     class SchedulePeriod < TimePeriod
       #------------------
       # ASSOCIATIONS
       #------------------
       has_many :closed_periods, :dependent => :destroy

       #------------------
       # INSTANCE METHODS
       #------------------
       #
       # Returns +true+ if the passed +date+ is in the
SchedulePeriod, but not in
       # any attached closed day ranges.
       #
       def include?( date )
         (start_date..end_date).include?(date) and
         not closed_periods.any? { |closed| closed.include?(date) }
       end
     end

Again the tests gave the green light.  All is good.

The second piece to this refactoring is to:  "Make the original
method abstract."  I considered that for a brief instance, mentally
translating the Javaism into Ruby.  My decision made, I highlighted
TimePeriod#include? and tapped the delete key on my keyboard.  I
briefly considered having it raise an exception, but Ruby is going to
do that for me anyway and this was less work.

The tests still pass.  Refactoring complete.

You may want to do another refactoring to move the duplicated date
range test (`(start_date..end_date).include?(date)`) out of the two
methods, but that's a different pattern and probably not too critical
in this example.

Hi Ruby Quiz,

Sorry, this is likely to be rather uninteresting, and I'm not even quite
following the rules, I think, but here is a refactoring that's not on
(Fowler's) official lists, even though it applies to Java as much as to
Ruby, that I did the other day; let's call them "Merge Redundant
Exception Handlers." Also, there's the more Ruby-exclusive "Remove
Unused Scope"

This code is from the Gem Server tutorial for win32/service on
RubyForge. It's by Daniel Berger, and refactored publicly without his
permission; I hope he doesn't mind.
http://rubyforge.org/docman/view.php/85/126/gemserver_tutorial.txt

   def service_main
      begin
         @s.mount_proc("/yaml") { |req, res|
            res['content-type'] = 'text/plain'
            res.body <<
Gem::Cache.from_installed_gems(File.join(Gem.dir, 
"specifications")).to_yaml
         }
      rescue Exception => e
         File.open(@logfile,'w+'){ |f| f.puts "Start failed: #{e}" }
         service_stop
      end

      begin
         @s.mount_proc("/") { |req, res|
            specs = []
            specifications_dir = File.join(Gem.dir, "specifications")
            Gem::Cache.from_installed_gems(specifications_dir).each do
|path, spec|
               specs << {
                  "name"           => spec.name,
                  "version"        => spec.version,
                  "full_name"      => spec.full_name,
                  "summary"        => spec.summary,
                  "rdoc_installed" =>
Gem::DocManager.new(spec).rdoc_installed?,
                  "doc_path"       => ('/doc_root/' + spec.full_name +
'/rdoc/index.html')
               }
            end
            specs = specs.sort_by { |spec| [spec["name"].downcase,
spec["version"]] }
            template = TemplatePage.new(DOC_TEMPLATE)
            res['content-type'] = 'text/html'
            template.write_html_on(res.body, {"specs" => specs})
         }
      rescue Exception => e
         File.open(@logfile,'w+'){ |f| f.puts "Start failed: #{e}" }
         service_stop
      end

      begin
         {"/gems" => "/cache/", "/doc_root" => "/doc/"}.each do
|mount_point, mount_dir|
            @s.mount(mount_point, WEBrick::HTTPServlet::FileHandler,
File.join(Gem.dir, mount_dir), true)
         end
      rescue Exception => e
         File.open(@logfile,'w+'){ |f| f.puts "Start failed: #{e}" }
         service_stop
      end

      begin
         @s.start
      rescue Exception => e
         File.open(@logfile,'w+'){ |f| f.puts "Start failed: #{e}" }
         service_stop
      end
   end


Refactoring 1: Merge Redundant Exception Handlers
/You have a series of exception handling blocks with identical 
handlers./
*Merge the code into a single exception handling block.*

Thus:

  begin
    foo
  rescue Exception => e
    barf e
  end
  begin
    bar
  rescue Exception => e
    barf e
  end

Becomes:

  begin
    foo
    bar
  rescue Exception => e
    barf e
  end

Refactoring 2: Remove Unused Scope
/You have a begin...end block that introduces a scope that is unused./
*Remove the unused scope.*

Thus:

  def foo
    begin
      bar
    rescue
      baz
    end
  end

Becomes:

  def foo
    bar
  rescue
    baz
  end

Here is the refactored code:

   def service_main
      @s.mount_proc("/yaml") { |req, res|
         res['content-type'] = 'text/plain'
         res.body << Gem::Cache.from_installed_gems(File.join(Gem.dir,
"specifications")).to_yaml
      }

      @s.mount_proc("/") { |req, res|
         specs = []
         specifications_dir = File.join(Gem.dir, "specifications")
         Gem::Cache.from_installed_gems(specifications_dir).each do
|path, spec|
            specs << {
               "name"           => spec.name,
               "version"        => spec.version,
               "full_name"      => spec.full_name,
               "summary"        => spec.summary,
               "rdoc_installed" =>
Gem::DocManager.new(spec).rdoc_installed?,
               "doc_path"       => ('/doc_root/' + spec.full_name +
'/rdoc/index.html')
            }
         end
         specs = specs.sort_by { |spec| [spec["name"].downcase,
spec["version"]] }
         template = TemplatePage.new(DOC_TEMPLATE)
         res['content-type'] = 'text/html'
         template.write_html_on(res.body, {"specs" => specs})
      }

      {"/gems" => "/cache/", "/doc_root" => "/doc/"}.each do
|mount_point, mount_dir|
         @s.mount(mount_point, WEBrick::HTTPServlet::FileHandler,
File.join(Gem.dir, mount_dir), true)
      end

      @s.start
   rescue Exception => e
      File.open(@logfile,'w+'){ |f| f.puts "Start failed: #{e}" }
      service_stop
   end

Cheers,
Dave

Dave Burt wrote:
> Hi Ruby Quiz,
> 
> Sorry, this is likely to be rather uninteresting, 

Actually, I find it very interesting ... but then, you have to consider 
my tastes. :)

> Refactoring 1: Merge Redundant Exception Handlers
> /You have a series of exception handling blocks with identical 
> handlers./
> *Merge the code into a single exception handling block.*
> 
> Thus:
> 
>   begin
>     foo
>   rescue Exception => e
>     barf e
>   end
>   begin
>     bar
>   rescue Exception => e
>     barf e
>   end
> 
> Becomes:
> 
>   begin
>     foo
>     bar
>   rescue Exception => e
>     barf e
>   end

You need to be careful here.  If 'barf' does not thrown an exception, 
but merely prints an error message of some kind, then the refactoring 
changes the behavior (which technically, makes it *not* a refactoring).

For example, from some (simplified) gem code I was working on just last 
week:

  def generate_docs
    begin
      generate_ri_docs
    rescue Exception => e
      puts "Problems in RI docs"
    end
    begin
      generate_rdocs
    rescue Exception => e
      puts "Problems in RDocs"
    end
  end

Merging the exception blocks in this case would mean that errors in 
generating the RI documents would skip the generation of the RDocs. 
Definitatly not a good thing.

--
-- Jim Weirich


> 
> Refactoring 2: Remove Unused Scope
> /You have a begin...end block that introduces a scope that is unused./
> *Remove the unused scope.*
> 
> Thus:
> 
>   def foo
>     begin
>       bar
>     rescue
>       baz
>     end
>   end
> 
> Becomes:
> 
>   def foo
>     bar
>   rescue
>     baz
>   end
> 
> Here is the refactored code:
> 
>    def service_main
>       @s.mount_proc("/yaml") { |req, res|
>          res['content-type'] = 'text/plain'
>          res.body << Gem::Cache.from_installed_gems(File.join(Gem.dir,
> "specifications")).to_yaml
>       }
> 
>       @s.mount_proc("/") { |req, res|
>          specs = []
>          specifications_dir = File.join(Gem.dir, "specifications")
>          Gem::Cache.from_installed_gems(specifications_dir).each do
> |path, spec|
>             specs << {
>                "name"           => spec.name,
>                "version"        => spec.version,
>                "full_name"      => spec.full_name,
>                "summary"        => spec.summary,
>                "rdoc_installed" =>
> Gem::DocManager.new(spec).rdoc_installed?,
>                "doc_path"       => ('/doc_root/' + spec.full_name +
> '/rdoc/index.html')
>             }
>          end
>          specs = specs.sort_by { |spec| [spec["name"].downcase,
> spec["version"]] }
>          template = TemplatePage.new(DOC_TEMPLATE)
>          res['content-type'] = 'text/html'
>          template.write_html_on(res.body, {"specs" => specs})
>       }
> 
>       {"/gems" => "/cache/", "/doc_root" => "/doc/"}.each do
> |mount_point, mount_dir|
>          @s.mount(mount_point, WEBrick::HTTPServlet::FileHandler,
> File.join(Gem.dir, mount_dir), true)
>       end
> 
>       @s.start
>    rescue Exception => e
>       File.open(@logfile,'w+'){ |f| f.puts "Start failed: #{e}" }
>       service_stop
>    end
> 
> Cheers,
> Dave

> If 'barf' does not thrown an exception,
> but merely prints an error message of some kind, then the refactoring
> changes the behavior (which technically, makes it *not* a refactoring).
Not only printing, anything that causes a side-effect.

--Meador

Meador Inge wrote:
>> If 'barf' does not thrown an exception,
>> but merely prints an error message of some kind, then the refactoring
>> changes the behavior (which technically, makes it *not* a refactoring).
> Not only printing, anything that causes a side-effect.

It's not about the side-effect at all (I think) it's about not exiting
the overall scope. I should stop posting in the wee hours.

This might be a correction, but it's 1:30am, so I can't be too sure.

  begin
    foo
  rescue Exception => e
    handler
  end
  begin
    bar
  rescue Exception => e
    handler
  end

Becomes:

  begin
    foo
    bar
  rescue Exception => e
    handler
  end

ONLY IF handler doesn't return or does throw an exception.

Does that hold? I'd like to get an actual re-factoring out of this :)

Oh, by the way,
Jim said:
> Actually, I find it very interesting ... but then, you have to
> consider my tastes.  :)

You're tastes are indeed strange, Jim!

Cheers,
Dave

Jim Weirich wrote:
>> handlers./
>>     bar
>>     barf e
>>   end
> 
> You need to be careful here.  If 'barf' does not thrown an exception, 
> but merely prints an error message of some kind, then the refactoring 
> changes the behavior (which technically, makes it *not* a refactoring).

What about using another method and yield here?

def barf_on_exception
  yield
rescue Exception => e
  barf e
end

barf_on_exception {foo}
barf_on_exception {bar}


Or even:

def on(ex, m)
  yield
rescue ex => e
  send m, e
end

on(Exception, :barf) {foo}
on(Exception, :barf) {bar}

But I'm not sure that it's really better than the original exception 
clause.

On 4/14/06, Ruby Quiz <james@grayproductions.net> wrote:
>
>
>         http://www.refactoring.com/catalog/index.html
>         New code
>         (optionally, unit tests created/used to verify the code)
>
> Submissions will be combined into an online catalog of Ruby Refactorings,
> probably on the RubyGarden wiki.
>
>

Extract method calls (
http://www.theserverside.com/articles/article.tss?l=AspectOrientedRefactoringPart1
)

Consider a cache that needs to be kept up to date.
For example, primitives in a complex vector scene:
redrawing the scene takes a relatively long time,
degrading framerate, but not redrawing the scene
when something changes is even worse. So I need
to detect when the cached rendering is outdated.

To do this, I keep track of the time of the last
rendering, and compare the vector object mtimes
to it. If a mtime is more recent than the last render
time, the scene needs to be redrawn.


Original code:

class VectorObject
  attr_reader :x, :y, :z, :path, :stroke, :fill, :mtime

  def x= x
    @x = x
    @mtime = Time.now
  end

  def y= y
    @y = y
    @mtime = Time.now
  end

  def z= z
    @z = z
    @mtime = Time.now
  end

  def path= pt
    case pt
    when Path
      @path = pt
    when Array
      @path = Path.new(pt)
    end
    @mtime = Time.now
  end

  ... and so on for stroke and fill

end


Detecting repetition, it makes code fragile and a pain to edit.
First pass, refactor `@mtime = Time.now` out to a method:

class VectorObject
  attr_reader :x, :y, :z, :path, :stroke, :fill, :mtime
  def touch!
    @mtime = Time.now
  end

  def x= x
    @x = x
    touch!
  end

  ... similarly for other accessors
end


A bit better. Still quite unwieldy though.
But! Metaprogramming to the rescue!
Alias the original methods to _non_touching and
replace them with a version that calls the original
method, followed by touch!, and returns what the
original method returned:

class VectorObject
  def self.touching! *mnames
    mnames.each{|mn|
      alias_method "#{mn}_non_touching", mn
      define_method(mn){|*args|
        rv = __send__ "#{mn}_non_touching", *args
        touch!
        rv
      }
    }
  end

  attr_reader :mname
  attr_accessor :x, :y, :z, :path, :stroke, :fill
  touching! :x=, :y=, :z=, :path=, :stroke=, :fill=
end


Finally, move #touching! out from the class and into a module:

module Touchable
  def touching! *mnames
    mnames.each{|mn|
      alias_method "#{mn}_non_touching", mn
      define_method(mn){|*args|
        rv = __send__ "#{mn}_non_touching", *args
        touch!
        rv
    }
  end
end


class VectorObject
extend Touchable
  attr_reader :mname
  attr_accessor :x, :y, :z, :path, :stroke, :fill

  def path= pt
    case pt
    when Path
      @path = pt
    when Array
      @path = Path.new(pt)
    end
  end

  touching! :x=, :y=, :z=, :path=, :stroke=, :fill=
end


Now we can use attr_accessor for creating
the simple methods, override #path= with
a magical setter, and not type touch! a lot.