880cbab435f00197613c9cc2065b4f5a

This smells... one of the main points of using has_finder was to isolate domain logic to the appropriate model (http://jamesgolick.com/2008/2/25/plugins-i-ve-known-and-loved-2-has_finder), but here it's scattered again.

Any ideas on how to elegantly clean this up?

class JobBoard < ActiveRecord::Base
  has_many :job_board_postings
  
  has_finder :distributed_by, lambda {|distributor_code| {:conditions => ['distributor = ?', distributor_code.to_s]} }
  has_finder :push,   :conditions => {:push => true}
  has_finder :manual, :conditions => {:manual => true}
  
  def push
    ! pull
  end
  alias_method :push?, :push
end

class JobBoardPosting < ActiveRecord::Base
  belongs_to :job
  belongs_to :job_board

  has_finder :distributed_by, lambda {|distributor_code| {:conditions => ['job_boards.distributor = ?', distributor_code.to_s], :include => :job_board} }
  
  # TODO: couldn't this be a delegated finder? not very DRY
  has_finder :pushed, :conditions => ["job_boards.pull = ?", false],  :include => :job_board
  has_finder :manual, :conditions => ["job_boards.manual = ?", true], :include => :job_board
end

Refactorings

No refactoring yet !

6c2573a4fc20f55bc18e620374e700b9

mynyml

April 9, 2008, April 09, 2008 22:58, permalink

2 ratings. Login to rate!

I've added a less fancy alternative, but I prefer the first one for two reasons:
1) forwards arguments, which means code doesn't need to be refactored if accessor object signature changes
2) keeps the 'delegation' semantics

'forwardable' is in stdlib

hope this helps

class JobBoard < ActiveRecord::Base
  has_many :job_board_postings
  
  has_finder :distributed_by, lambda {|distributor_code| {:conditions => ['distributor = ?', distributor_code.to_s]} }
  has_finder :push,   :conditions => {:push => true}
  has_finder :manual, :conditions => {:manual => true}
  
  def push
    ! pull
  end
  alias_method :push?, :push
end
require 'forwardable'
class JobBoardPosting < ActiveRecord::Base
  belongs_to :job
  belongs_to :job_board

  class << self
    extend Forwardable
    def_delegator  JobBoard, :push, :pushed
    def_delegators JobBoard, :manual, :distributed_by
  end
end
# ...
  class << self
    def pushed; JobBoard.push end
    def manual; JobBoard.manual end
    def distributed_by(code); JobBoard.distributed_by(code) end
  end
# ...

Your refactoring





Format Copy from initial code

or Cancel