Code Quality Catch-22 -- Duplication vs Complexity I've started using Code Climate to assess some of my code quality and ran into a situation that I can't seem to find a good solution for. In short, in trying to solve some code duplication that resulted in a low "grade" by using metaprogramming, I simply get a low grade for code complexity instead.

Anyone know a good a way out of this trap?



Code Quality Background Info

Here are some resources that point to the underlying libraries used by Code Climate to assess code quality. I think these are pretty standard in the ruby world so this  isn't a Code Climate specific question.

Deciphering Ruby Code Metrics

Tools used by Code Climate

Before Code (duplication problem)

Several associations on User that have basically identical functionality.

  has_many :user_conditions
  has_many :conditions, :through =&gt; :user_conditions do
    def &lt;&lt;(new_item) # disable duplicate addition
      super( Array(new_item) - proxy_association.owner.conditions )
    end
  end
  def activate_condition(condition)
    ActiveRecord::Base.transaction do
      self.conditions &lt;&lt; condition
      self.update_attribute(:active_conditions, (self.active_conditions | [condition.id.to_s]))
    end
  end
  def deactivate_condition(condition)
    self.update_attribute(:active_conditions, (self.active_conditions - [condition.id.to_s]))
  end

  has_many :user_symptoms
  has_many :symptoms, :through =&gt; :user_symptoms do
    def &lt;&lt;(new_item) # disable duplicate addition
      super( Array(new_item) - proxy_association.owner.symptoms )
    end
  end
  def activate_symptom(symptom)
    ActiveRecord::Base.transaction do
      self.symptoms &lt;&lt; symptom
      self.update_attribute(:active_symptoms, (self.active_symptoms | [symptom.id.to_s]))
    end
  end
  def deactivate_symptom(symptom)
    self.update_attribute(:active_symptoms, (self.active_symptoms - [symptom.id.to_s]))
  end

  has_many :user_treatments
  has_many :treatments, :through =&gt; :user_treatments do
    def &lt;&lt;(new_item) # disable duplicate addition
      super( Array(new_item) - proxy_association.owner.treatments )
    end
  end
  def activate_treatment(treatment)
    ActiveRecord::Base.transaction do
      self.treatments &lt;&lt; treatment
      self.update_attribute(:active_treatments, (self.active_treatments | [treatment.id.to_s]))
    end
  end
  def deactivate_treatment(treatment)
    self.update_attribute(:active_treatments, (self.active_treatments - [treatment.id.to_s]))
  end


After Code (complexity problem)

Moved out of User into a module, admittedly this code is hard to grasp.

module UserTrackables
  extend ActiveSupport::Concern

  included do |base|

    %w( treatments conditions symptoms ).each do |trackable|

      base.class_eval do
        has_many "user_#{trackable}".to_sym
        has_many trackable.to_sym, :through =&gt; "user_#{trackable}".to_sym do

          # disable duplicate addition
          def &lt;&lt;(new_item)
            super( Array(new_item) - proxy_association.owner.send(proxy_association.reflection.plural_name.to_sym) )
          end

        end
      end

      define_method("activate_#{trackable.singularize}") do |trackable_object|
        ActiveRecord::Base.transaction do
          self.send(trackable) &lt;&lt; trackable_object
          self.update_attribute("active_#{trackable}".to_sym, (self.send("active_#{trackable}") | [trackable_object.id.to_s]))
        end
      end

      define_method("deactivate_#{trackable.singularize}") do |trackable_object|
        self.update_attribute("active_#{trackable}".to_sym, (self.send("active_#{trackable}") - [trackable_object.id.to_s]))
      end

      define_method("current_#{trackable}") do
        trackable.singularize.capitalize.constantize.where(id: self.send("active_#{trackable}".to_sym).map(&amp;:to_i))
      end

    end
  end

end
Your first version - extending the association - is probably the right approach in general, but there's another way to remove duplication, without resorting to metaprogramming.

Instead of defining methods as inlined extensions you can create a module for the association extensions. But rather than adding that module to the User model, you can add it the associations instead (see the docs)

E.g. if you create a module like so:

module TrackableAssociation
  def &lt;&lt;(item)
    # ...
  end

  def activate(item)
    # ...
  end

  def deactivate(item)
    # ...
  end
end


You should be able to add it like so:

class User &lt; ActiveRecord::Base
  has_many :conditions, through: :user_conditions, -&gt; { extending TrackableAssociation }
  has_many :symptoms, through: :user_symptoms, -&gt; { extending TrackableAssociation }
  has_many :treatments, through: :user_treatments, -&gt; { extending TrackableAssociation }
end


Which, in turn, should let you do things like

user.conditions.activate(condition)


Now your extensions are defined without using duplication or metaprogramming.

However. Your approach of storing an array of "activated IDs" smells fishy. It's fragile, and it requires the association extensions to modify the user instance. The code paths get tricky.

active should be an attribute on a join model - or at least a column in the join table. If I'm reading your code right, you've got Condition, Symptom and Treatment models, and join tables between them and User. You should probably make actual models for the join; said models can then have an active boolean attribute.