# 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.

## 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

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


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.

• 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 '15 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 '15 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 '15 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 '15 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 '15 at 9:33