views:

188

answers:

3

Currently I have code like the following (simplified somewhat). Eventually, I've added more and more new classes like D1/D2, and I think it's time to do some refactoring to make it more elegant. The goal of course is to make adding new class Dx use as little duplicate code as possible. At least, the duplicate parts of calling FileImporter.import inside the singleton method Dx.import should be factored out.

module FileImporter
  def self.import(main_window, viewers)
    ...
    importer = yield file  # delegate to D1/D2 for preparing the importer object
    ...
  end
end

class D1
  def self.import(main_window)
    viewers = [:V11, ]  # D1 specific viewers
    FileImporter.import(main_window, viewers) do |file|
      importer = self.new(file)
      ...  # D1 specific handling of importer
      return importer
    end
  end
end

class D2
  def self.import(main_window)
    viewers = [:V21,:v22, ]  # D2 specific viewers
    FileImporter.import(main_window, viewers) do |file|
      importer = self.new(file)
      ...  # D2 specific handling of importer
      return importer
    end
  end
end

# client code calls D1.import(...) or D2.import(...)

Essentially FileImporter.import is the common part, with Dx.import being the variation. I'm not sure how to refactor these singleton methods. What is the common Ruby way of doing this?

Update: (some comments were added into code above, hopefully make my intension clearer ...)

Originally, I've left out code I thought not significant to avoid confusion. I should have mentioned that the code above was also the result of refactoring class D1 and D2 (by moving common part out and into module FileImporter). The purpose of D1.import and D2.import was mainly to create objects of proper class (and possibly followed by some class-specific handling before returning from the block). FileImporter.import is mainly the common logic, within which at some point would yield to specific class for generating the importer object.

I feel that class D1 and D2 and looks really similar and it should be possible to further refactor them. For example, they both call FileImporter.import to supply a block, within which both create an object of itself.

Solution: Originally I didn't realize you could call base class's singleton methods simply by calling super from within derived class's corresponding singleton methods. That was really the main problem I had and was not able to go with that route. So I've accepted @makevoid answer as it indeed makes creating new derived classes more easily.

Using a common base class is an elegant refactoring solution, but one problem with that is all new derived classes would already use up the one base class quota. I've came to this class macro method which provides even more concise results from the derived class perspective.

module FileImporter
  def self.included(mod)
    mod.extend ClassMethods
  end

  module ClassMethods
    def importer_viewer(*viewers, &blk)
      @viewers = viewers
      @blk = blk

      class << self
        def import(main_window)
          if @blk.nil?
            FileImporter.import(main_window, @viewers) do |file|
              self.new(file)
            end
          else
            FileImporter.import(main_window, @viewers, &@blk)
          end      
        end
      end
    end
  end

  def self.import(main_window, viewers, multi=true)
    ...
    importer = yield file  # delegate to D1/D2 for preparing the importer object
    ...
  end
end

class D1
  include FileImporter
  importer_viewer [:V11, ] do
    ...  # D1 specific handling of importer
  end
end

class D2
  include FileImporter
  importer_viewer [:V21,:v22, ] do
    ...  # D2 specific handling of importer
  end
end
+1  A: 

Maybe it's not the best solution but at first seems that Dx classes share the same behaviour, so sublassing them with a C class that has a self.import method that uses a block to accept some other code could work. Or that could be done by including a module too.

Anyway, something like this should work (sorry for the shorter names but were good for prototyping). Also notice I changed FileImporter.import method name to another to avoid misunderstandings and note that i haven't tested the code :)

module F
  def self.fimport(something)
    yield "file"
  end
end


class C   
  include F 

  def initialize(f)

  end

  def self.import(something, &block)
    F.fimport(something) { |f|
      d = self.new(f)
      block.call
      d
    }
  end
end


class D1 < C
  def self.import(something)
    super(something){
      puts something
    }
  end
end


class D2 < C
  def self.import(something)
    super(something){
      puts something
    }
  end
end

p D1.import("a")
p D2.import("b")

#=> a
#=> #<D1:0x100163068>
#=> b
#=> #<D2:0x100162e88>
makevoid
This is a good solution. However, I get a feeling that it is more like a 'Java' way, that is, type hierarchy coupled with block/closure. I'm wondering if Ruby people do things differently.
bryantsai
A: 

It seems like making a module would be an elegant solution. However, it's hard to say with a vague idea of what the code is for. Example:

module Importer
  def import
    self.whatever # self should be D1 or D2 as the case may be
    # ...
  end
end

class D1
  include Importer
end

class D2
  include Importer
end
Benjamin Oakes
The refactoring targets were singleton methods.
bryantsai
Do you mean class methods? This technique can work for those too. (Using `extend` instead of `include`.)
Benjamin Oakes
There are variations in `D1.import` and `D2.import`. With this method of module extend, how and where would their variations put?
bryantsai
Well, the way to approach the problem really depends on what you need to do. From the example, it doesn't look like there are any variations in `D1.import` and `D2.import`. More context/explanation seems necessary to be able to really have a solution that makes sense.
Benjamin Oakes
A: 

Given the limited code and context given, I suspect that something like the following would work for you. If nothing else, you can get idea of how to use Modules to break out common functionality.

module FileImporter
  def self.internal_import(main_window, viewers)
    ...
    importer = yield file
    ...
  end
  private :self.internal_import
end

class D1
    include FileImporter
    def self.import(main_window)
      self.internal_import(main_window, [:V1, ])
    end
end

class D2
    include FileImporter
    def self.import(main_window)
      self.internal_import(main_window, [:V2, ])
    end
end
bta