Would you merge repeating code even when it gets more complex?

Ruby
1

Hi, I wrote a module including some instance and class methods with are
very similar. That means some lines do exactly the same once for
instance and once for class methods. I started to refactor theses
methods but the code got quite complex as a result.

Please take a look at this module:

https://github.com/mjio/uberhook/blob/master/lib/uberhook/base.rb#L43

Would you merge these methods to remove the repeating code even when the
code gets more complex and harder to read?

2

Martin J. wrote in post #1073029:

Hi, I wrote a module including some instance and class methods with are
very similar. That means some lines do exactly the same once for
instance and once for class methods. I started to refactor theses
methods but the code got quite complex as a result.

Please take a look at this module:

https://github.com/mjio/uberhook/blob/master/lib/uberhook/base.rb#L43

Would you merge these methods to remove the repeating code even when the
code gets more complex and harder to read?

I haven’t looked to thoroughly but for example this calls for
simplification:

  def run_instance_callback(type, target, scope)
    return unless instance_hooks[target].has_key?(type)
    scope.instance_eval(&instance_hooks[target][type])
  end

  def run_class_callback(type, target, scope)
    return unless class_hooks[target].has_key?(type)
    scope.instance_eval(&class_hooks[target][type])
  end

→ 

  def self.run_callback(type, target, scope, hooks)
    return unless hooks[target].has_key?(type)
    scope.instance_eval(&hooks[target][type])
  end

→ 

  def self.run_callback(type, target, scope, hooks)
    tp = hooks[target][type] and scope.instance_eval(&tp)
  end

Looking a bit further it seems you want to put this in a class
hierarchy:
Hooks
ClassHooks < Hooks
InstanceHooks < Hooks

Then apply “template method pattern” e.g. for the different conditions
in method instance_hook and class_hook.

Pushing this further you then only retain method instance_hooks and
class_hooks which return a class of the corresponding type. Instead
calling instance_hook(…) {|…|} someone will invoke
instance_hooks.hook(…) {|…|}. You could as well keep instance_hook
but create forwarding in a simple way, e.g.

%w{hook process_method_added}.each do |name|
%w{class instance}.each do |scop|
class_eval “def #{scope}_#{name}(*a,&b)
#{scope}_hooks.#{name}(*a,&b) end”
end
end

Kind regards

robert

3

Uncle Bob commands: extract methods till you drop. Extract your repeated
code until there isn’t any.

Now, I try not to be religious about this, but I can’t think of any
examples where having repeated code has been anything other than a
maintenance problem. If this is personal code, do whatever. If it’s for
a company , Bob Martin’s advice is well worth considering.

4

On Thu, Aug 23, 2012 at 10:34 PM, Martin J. [email protected]
wrote:

–
Posted via http://www.ruby-forum.com/.

I think the problem here is confusion about object system. I don’t see
any
reason to explicitly handle instance methods and class methods. When you
remove this requirement, the duplication goes away.

I was going to try refactoring it to show you what I mean, but the tests
can’t accommodate that, they test implementation instead of behaviour
(each
method is explicitly tested, so if I don’t have those methods doing
those
exact things, then I’ll fail the tests). So instead, I wrote an example
from scratch: Creates before/after hooks for methods · GitHub