How much ‘less code’ is better?
George Malamidis, February 13th, 2007
Let’s consider this piece of neat meta-programming sugar. We have two Rails Models, User and Role with Migrations defined as:
class CreateUsers < ActiveRecord::Migration
def self.up
create_table :users do |t|
t.column 'role_id', :integer
t.column 'email', :string
end
end
def self.down
drop_table :users
end
end
class CreateRoles < ActiveRecord::Migration
def self.up
create_table :roles do |t|
t.column 'name', :string
end
end
def self.down
drop_table :roles
end
end
Let’s imagine that we have three types or Role, Root, Administrator and Customer. We want the Users to know which Role they belong to. Supposing Role looks as simple as:
class Role < ActiveRecord::Base has_many :users end
… a perfectly acceptable implementation of User could be:
class User < ActiveRecord::Base
belongs_to :role
def root?
self.role.name == 'root'
end
def administrator?
self.role.name == 'administrator'
end
def customer?
self.role.name == 'customer'
end
end
Now, I get an uneasy feeling every time I see similar looking code repeated more than once and, thanks to Ruby’s monkey code remedy nature, I can achieve the same result by writing the above as:
class User < ActiveRecord::Base
belongs_to :role
Role.find(:all).each do |role|
define_method("#{role.name}?") do is? role.name end
end
private
def is? role_name
self.role.name == role_name
end
end
There’s another advantage to this approach. Come a time when I need to add more Role definitions, I will not have to worry about User, in terms of writing code for it to comply with the changes to Role.
But all this begs the question: Is every body better off with me trying to be clever and not doing the straightforward thing any ol’ dev could grasp in a second, even though I’m saving the world (and my app) a few bytes?
To be honest, I’m not particularly fond of the idea of compromising code effectiveness or elegance in order to accommodate mediocre developers. I find that to be a pessimistic strategy that could ultimately act against the benefit and integrity of the solution.
Having said that, being part of larger teams exponentially increases one’s responsibility towards the possible maintainers of their code, so I don’t feel I’m 100% right on the above declaration.
February 13th, 2007 at 10:59 pm
I don’t think that code is too much magic. The methods are still defined in the User class. Any competent developer should be able to figure that out from looking at that code. It’s not like when programming in ruby you have an IDE for you where you could click on “user.customer?” and have it take you to the code where that is defined.
One thing I would be worried about with the above code is name collisions. For example, what if you add a role called foo and then a method called “foo?”. you might run into some strange errors that are difficult to debug. Maybe make the auto_generated methods like user.role_customer? to avoid that?
The area where I get really worried about having to much magic in the code is when you have some random code off in some other file that adds methods to a class.
February 14th, 2007 at 8:34 pm
It’s true, ‘any competent developer could figure it out’ — The problem is the length of time it takes the developer to figure it out.
I have recently been looking through a lot of code, all which can be ‘figured out’. I keep on thinking that it would be so nice if for each class there was some sort of human readable description that told me in general terms what the class does. Ah-HA! Documentation!!
If you were to provide a one or two sentence description of what the clever code does in human readable terms, then you have gained both cleverness and readability.
February 14th, 2007 at 9:06 pm
I understand your point, programmers are not islands and the source code is part of social network.
February 15th, 2007 at 6:45 pm
This is an argument for the use of comments. I know that they went out of fashion some time ago, but absolutism sucks.
February 17th, 2007 at 11:57 pm
The code is certainly neater, however there is an argument that the interface of User is now unclear - we only know it responds to an administrator? method because there is a role in the data with the name “administrator”, and if that role name was to change or the role removed, then the interface to your class would also magically change - with possible undesirable effects on it’s clients.
If the concept of these particular roles is required in the domain model, and is actively used by code (such as the client of User which is invoking administrator?), then I feel they should be represented directly in the code, possibly as an enumeration (and then these convenience methods could be defined by iterating over the enumeration). Of course this then begs the question of why Roles, now of fixed values from an enumeration, would be in the database at all.
If, on the other hand, they are in the database because they are configurable, then it is probably better to not have these convenience methods at all - but rather to provide a single is? or inRole? method that takes a Role object (not a role name). The client that does care about particular Roles, like administrator, can then obtain the Role object it wants, and then invoke the is? method with that role. This allows the failure to find a particular role in the data to be indicated at precisely the point where the code is dependant on that role existing.