Stupid Easy: 7 Deadly [Rails] Sins, Part 1

Written on 10:46:00 AM by S. Potter

Everyone has been talking about Rails for a few of years now as the framework that makes it "stupid easy" to create web applications. The problem I have with this mentality is it creates a "stupid easy" sub-culture within the Rails community that promotes stupid laziness (as opposed to smart laziness). Over the last 3 Rails contracts, I have worked with supposedly experienced developers that have switched to Rails from OO languages like Java and C++ (as opposed to pretend OO languages like PHP). The results have been VERY disappointing. The biggest problem I see is that some, even quite experienced developers, have the [stupid] lazy mentality burned into their brains. They want to visit blogs and copy and paste code into their applications without thinking about it. In fact, recently I found a ridiculous real world example of such madness. No thought put into the pasted code AT ALL. I could identify the blog posting on a popular Rails blog that had the EXACT same code. The variables weren't EVEN renamed to be meaningful in the current application. Instead a finance web application had references to customers and invoices in the controller and view code when there were no such entities/models involved in the application. Not only is this a maintenance nightmare, it show how little thought went into the code just to get a small AJAX effect that (a) wasn't difficult to write from scratch (4 lines in total after refactoring from the 11 lines of code used) and (b) was probably not even that necessary from the user experience perspective. These are the developers I think ought to return to their no-thinking PHP or Java/JEE recipe books and leave the Ruby world alone. Of course, the authors of "recipe" books should also bare some of the blame for encouraging the no-thinking hackers out there to join the Ruby world in the first place. Is that really the type of mindset we wish to harbor in our community? Look at the PHP and Java universes today for a reference point of where we will end up very soon if we are not careful.

1. Stupid Easy Schemas

One of the deadly sins I saw on my travels through their code was extending the "stupid easy" mentality in the form of not defining schema properly. Sure they *used* Rails' migrations, but they didn't really create a usable schema based on the use stories and cross-story functional requirements. While you might not want to use database specific features like triggers, stored procedures, or even foreign keys (the latter assumes you are using Rails-based equivalents in place of FKs), you should still make your schema sensible. For example,

# Migration code
  create_table :countries do |t|
    t.column :code, :string
    t.column :name, :string
  end

# Model code
class Country < ActiveRecord::Base
end
In this example we created a table called countries that has two string fields: code, name. In this case the code is the ISO country code that the application is going to use for almost all country lookups in the database. We knew that pretty much from the beginning because of the nature of our application. (Note: this example is a little contrived as I had to change things so as not to expose too much about the application this is from). Not only do we know that the Country model will almost always be looked up by its code (from the initial set of use(r) cases/stories we need to implement). The ISO code is also always 3 characters long. We also know that these ISO codes are unique for each country. All these things we knew before we needed to create the schema. This is the important information that is missing from the schema and the model is missing relevant validation. Before committing this new model and migration for Country the developer writing the code (one of the developers I am thinking of loves to tout how XP/agile and great his code is), should have had the following code on initial checkin (there really is no excuse IMHO):

# migration code
  create_table :countries do |t|
    t.column :code, :string, :limit => 3
    t.column :name, :string # You may even decide to cap the length of the name field too, but this is more of a DBA style thing.
  end
  add_index :countries, :code, :unique => true

# Model code
class Country < ActiveRecord::Base
  validates_length_of :code, :is => 3
  validates_uniqueness_of :code
end
While on my soapbox about this point, another developer I respect said this might be premature optimization. In this specific case, I totally disagree, however, this is a good point to make in general. You don't want to do premature optimization either. This is something we all need to be careful about and to be aware. This is the point where you need to think for yourself. There is no cheat sheet on these types of thinking points. The [stupid] lazy Rails developers should go back to hacking PHP senselessly or following J2EE/JEE blueprint patterns if they find thinking for themselves to develop their own rules of thumb too much work. Remember [smart] lazy is the way to go.

2. Stupid Easy Models

Another cardinal sin I often see in both client work I have inherited from others or open source projects is dumbing down models. Instead of creating member methods of models that related to *what* they are and do, some Rails coders (usually not very experienced) write this functionality within the controller layer, which leads to drastic controller layer bloat. Which is really ugly. Remember if your code is starting to look ugly, do something about it. The way to fix this is *almost* always to create relevant member methods on the corresponding models that this functionality works on. One easy example of this might be to add an authenticate method to the User model instead of having extra logic in the ApplicationController. Think CRC instead of procedural.

3. Stupid Easy Controller Methods

This is perhaps the most obviously harmful (from a security perspective) sin. While traveling through the Supid Easy Rails universe of code, I almost always see unprotected controller methods that are used as filters or for utility purposes in some capacity. This is a sin that I myself must confess to, though I haven't done so since the first 3 months of my Rails development around 2005 Q4. The following is typically what I see around:

# in SparksController
  def check_permissions
    return false unless current_user.has_permission?(:MANAGE_USERS)
    true
  end
The problem with this is that we can do the following (unless using resource routes where default routes are taken out):
GET /sparks/check_permissions?some_var=some_value
And perhaps hack into the web application. In this case we may not be able to do anything too interesting, but there are other scenarios that this could create a very open hole in the web application. The way to fix this and keep your team's sanity from a maintenance perspective is to protect these filter or utility methods by scoping appropriate with protected or private, which is not difficult at all. However, if you are a true Rubyist you will probably opt to keep your controller utility methods in Acts::As mixins or similar.

If you enjoyed this post Subscribe to our feed

No Comment

Post a Comment