current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 3 down vote favorite

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 => :user_conditions do
    def <<(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 << 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 => :user_symptoms do
    def <<(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 << 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 => :user_treatments do
    def <<(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 << 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 => "user_#{trackable}".to_sym do

          # disable duplicate addition
          def <<(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) << 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(&:to_i))
      end

    end
  end

end
share|improve this question

1 Answer 1

up vote 4 down vote accepted

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 <<(item)
    # ...
  end

  def activate(item)
    # ...
  end

  def deactivate(item)
    # ...
  end
end

You should be able to add it like so:

class User < ActiveRecord::Base
  has_many :conditions, through: :user_conditions, -> { extending TrackableAssociation }
  has_many :symptoms, through: :user_symptoms, -> { extending TrackableAssociation }
  has_many :treatments, through: :user_treatments, -> { 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.

share|improve this answer
    
Thanks @Flambino, I like this idea of extending the association. I agree that the active stuff is fishy, and it may well be changed. Overall though, do you think your answer suggests that metaprogramming is bad for a healthy codebase? Or maybe just in certain cases? Or only as a last resort? –  The Worker Ant Jan 18 at 21:52
    
@TheWorkerAnt Metaprogramming definitely has its uses (Rails uses it in many places) but everything in moderation, as they say. It really depends on the context, but I'd say that if a neat non-meta approach exists (like here), it's probably the better one. The occasional use of send is probably ok, if the alternative is a ton of duplication. Using define_method is more icky. Use both, and the code quickly goes from plain language to abstract algebra. While neat, it does make it harder to work with. And remember: Code is read much more often than its written. –  Flambino Jan 19 at 0:21
    
@TheWorkerAnt By the way: It's funny, one of my first questions on this site was about CodeClimate giving me a B for using metaprogramming :) Years later, and I'm still of the opinion that my case for using it was sound, as it avoided some truly tedious duplication. But perhaps a completely different solution existed as well that wouldn't require duplication or metaprogramming. In short, if the choice is between duplication and metaprogramming, consider if the answer is "neither". –  Flambino Jan 19 at 0:28
    
Okay that makes me feel a bit better about this situation. CodeClimate support said basically "don't worry about it, trust your gut". But that's hard to swallow when you've got a big orange D sitting there. I think your solution will fix that though, and I'll pray I never get caught in this situation again. –  The Worker Ant Jan 19 at 12:16
    
@TheWorkerAnt You probably will face a similar situation again. If there's never a challenge to programming, it wouldn't be much fun, would it? :) Anyway, "trust your gut" really is the best advice. The most important bit is that you actually listen to your gut in the first place, so you at least know when something's not right. –  Flambino Jan 20 at 9:33

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.