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

Written on 9:23:00 AM by S. Potter

This is the third and final part in the Stupid Easy: 7 Deadly [Rails] Sins series. See part 1 and part 2 if you haven't already. I have left the most heinous sin to the end. If you are a serious sinner in this aspect, there is little hope for you unless you take steps now to rectify your behavior in this aspect.

Stupid Easy Ruby

When in Rome, do as the Romans do
That's right. This is the sin of not knowing how to really code in Ruby. Sure some coders that move to Ruby can write code that does what they want it to do, but often at the severe expense of compromising project code maintainability (e.g. making things very unDRY, etc.) or even introducing significant performance issues by not understanding basics about the Ruby language. Instead I see people using Java, Perl, PHP, C# or even Python idioms in Ruby instead of understanding the essence of the language of Ruby, which is distinct from each of these languages in different ways. The same is true for any language/environment you work within. I know former PHP people that moved to Java and do not have the first clue about basic Java idioms that work well. This isn't just to pick on PHP heads as I imagine this occurs from any language migration path. I have simply seen this sin committed most by former PHP coders moving toward Java, Ruby and Python. However, I will say I see a LOT of former Java (massive static language proponents - Joshua Bloch you are a nonobjective snob) developers that do not understand or appreciate the non-static design mentality and they create large class hierarchies and don't understand what a Mixin is (or they pretend to be "cool" and talk alot of about these idioms, but don't really utilize them in the right way themselves). The Ruby mindset is still different from it's more similar looking cousins: Python and Perl, but moving between Python, Perl and Ruby (in any direction) feels more intuitive (at least in my mind). Now I should stress that I can no longer code in Java without pulling my hair out because the thought of creating 5 interfaces and a factory for every three class implementations drives me temporarily insane. The SPI design principle allows Java to be fairly flexible (especially for a statically typed language), but at the expense of my personal sanity now that I prefer to think the non-static way (yes, I still appreciate Python's strongly-typed ways). This is why I prefer to use JRuby, Jython or Groovy scripting in Java environments. There is no magic bullet to stop sinning in this regard if you aren't willing to take a journey to learn and understand Ruby idioms (not just basic *knowledge*). There are no shortcuts, it simply depends on how easily your brain can shift in gears. This doesn't mean people who can't shift gears are "bad" developers, but not well suited to migrating to significantly different environments very often. Just remember the rules in Ruby aren't the same. Think in terms of sending messages to objects that may or may not respond to the message you send, instead of expected interfaces and you are half way there if you are coming from Java, C++ and similar languages. Think of Java as a big government structure with a complicated tax code and Ruby as minimal government with a simplified tax code without loopholes accessible only to the rich! That means in the Ruby world wild things such as adult services can legally be procured between consenting adults, so bend your brain to think that way when working in Ruby...almost anything is possible especially in the realm of metaprogramming that is not possible in the Java world.
That government is best which governs least.
--Thomas Paine
I personally happen to agree with Thomas Paine's statement (at least in the context of programming), but there are various programming philosophies that different languages cater for, so it comes down to your own preference. Pick the ones you can live with at the time and think in terms of and you will be happier for it. Obviously in a programming context some people (e.g. Joshua Bloch, who is still a static language snob) will oppose legal procurement of "adult services". They have a moral objection to these activities and thus do not appreciate the beauty of dynamic design. If you have such moral reservations in programming, do everyone a favor and stay in the Java world or at least static languages! This is the beauty of different opinions. Nobody is right in absolute terms, but you need to make the right choice for yourself. It is the zealots who only accept "absolute correctness" that we should be weary of (on any side of the argument, but including Joshua Bloch). My general philosophy when I had the Java Way ingrained in my brain was that as an API designer I had to protect stupid developers from their own stupidity. Now my philosophy in the Ruby world is that I write APIs for smart developers and if you aren't [Ruby] smart, use at your own risk. Of course, I oppose complication for the sake of appearing to be "smart". But I try to write APIs that allow more advanced Ruby developers to take advantage of language features such as blocks, metaprogramming, etc. as opposed to providing an over-simplified, dumbed-down API that ends up looking ugly to the wiser Ruby developer due to being less DRY than might be possible without restriction.

Ruby Language Pointers

If you haven't acquainted yourself with the pickaxe book you should pay special attention to the following sections: Blocks, Mixins, Inspecting Objects, Inheritance & Messages, Inheritance & Mixins, Classes & Objects Interacting, The Ruby Language. Then I would look at the source code of the ActiveSupport open source project (part of Rails) for techniques that are very useful. Some of these techniques I have discussed in previous blog posts: Ruby Idiom: Reopening Classes, Ruby Idiom: forwardables, Ruby Idiom: Inject-ing Understanding, Ruby Equality, Ruby Higher Order Messaging.

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

Written on 9:43:00 AM by S. Potter

In the second part of Stupid Easy: 7 Deadly [Rails] Sins, we look at sins four through six and save the most heinous sin for last in the third part of this series.

4. Stupid Easy Views

Everyone has seen spaghetti view code, either in the JSP, PHP, or other similar web templating environments. So why do people honestly think (ok, only the no-thinkers) that when they use Rails that you can't violate MVC because it is built around the MVC "pattern". Don't even get me started on the whole "pattern" thing... If you see a piece of view code like the following, then you are violating MVC. Yes, even when using Rails:

<% 
if current_user && current_user.admin?
  @somevar = something goes here
  # and do something else here
else
  @somevar = something else goes here
end
%>
This is a small (but very stupid) violation, but a violation none-the-less. I have seen not just logic in view code, but oddly model-specific code (in one case I even saw some attributes of a model being updated at the END of the view template - go figure). Now simply checking the admin flag of the current_user to insert some view specific code, would not be violating MVC. There are various ways of making sure you don't make this mistake. Some people think using a specialized templating language in place of eRB is the way to go (one of the many potential replacements that I happen to know about is Haml). These specialized template languages basically make it virtually impossible to embed any kind of logic into the view code. Others (pro-thinkers, rather than no-thinkers) don't believe that level of enforcement is necessary and just make a point of highlighting to the team (and then quickly refactoring) any violations in view code to educate all the team members about this sin. I have even seen some Rake tasks written (ok, I may have written a couple) that flag any views that have specific keywords within the <% %> eRB brackets as potential violators so that we can look through the suspects and make judgments before a release is made (because you never want to release code that violate basic principles you believe in). Another strategy is to only work with developers that you share a very similar development philosophy and know they wouldn't violate this basic rule of thumb in the first place. In any case, you need to incorporate one of these strategies so you can be sure to be rid of the Stupid Easy View faux pas in Rails projects.

5. Stupid Easy Configuration (and Routes)

In Rails 2.0 there is absolutely ZERO excuse for this (with very nice initializer hooks available), but even pre-2.0 it is hard to justify an config/environment.rb consisting of more than 30 lines (including basic Rails configuration minus comments). Separation of configuration into separate files that are required by config/environment.rb were my personal preference in pre-2.0. On the config/routes.rb side, I would often see the default routes still uncommented. In Rails post-1.2 there is really no excuse since almost everyone should be using RESTful routes with RESTful controllers (even if the way Rails generates a ridiculous amount of code, when it could be refactored nicely into a mixin or base class, is pretty dumb) anyway. In fact, default routes are almost always an invitation for troublemakers to hack into your site (especially if they know it is a Rails site). I can't say I have never deployed a Rails 1.2.x site with default routes uncommented, but I certainly haven't in the last year since Rails 1.2's official release.

6. Stupid Easy Library Code

For some reason numerous popular Rails plugins promote the idea of putting generated files into your RAILS_ROOT/lib directory (e.g. acts_as_authentication, restful_authentication, etc.). Instead of being a real plugin they are basically just Rails generators. Now we could debate how generators could be better supported in Rails, but that is really a tangent to the point. Don't be fooled by these popular "plugins" (remember they are really only generators). If you have significant code in your RAILS_ROOT/lib that has the same focus (e.g. authentication, authorization, credit card processing, etc.), then you really ought to create your own plugin (even if it isn't going to be shared with other projects in the foreseeable future). How you and/or your team defines "significant" is a judgment call, but I personally think any unit of code that works together that is greater than 200 lines of elegant Ruby code is significant (minus empty lines and comments of course). It isn't necessarily a hard or fast rule, but a general rule of thumb, which may have exceptions. The main purpose of this is twofold from my perspective. When you have a lot of code in your RAILS_ROOT/lib it is a harder to be more disciplined about organization. Whereas with a plugin you know where plugin initialization code goes vs. mixin or other code. Another big benefit (which is related to code organization) is testing/specing. It is much easier to see (or specifically notice) test/spec-coverage shortcomings of plugins than holes in your tests/specs for RAILS_ROOT/lib code. Benefits of creating plugins include that it is forces you (the developer) to think more about:
  • Testing/specing your code
  • code organization from a maintainability perspective
  • what chunks of code go together (should this be one or two plugins)
  • the scope of your plugin vs. what needs to be defined or written in your application code
  • who is responsible for developing this functionality, maybe it is the infrastructure/core/framework development team who needs to be responsible for authentication or authorization code rather than application developers. With plugins you can separate our these responsibilities very easily
In addition, a plugin is much more easily shareable across projects (in a more organized and disciplined way than just copying or even linking files/directories into your RAILS_ROOT/lib directory). Of course, I also find the way Rails plugins are supported by Rails a hindrance, especially when RubyGems could provide a many facilities to support this in a more elegant way, but that is a topic for another day!:) There is only one Stupid Easy Deadly [Rails] Sin left, which I will publish in the next two weeks.

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.

'Twas the night before launch...

Written on 7:10:00 PM by S. Potter

Oh fun times! On such fateful nights you know you will find something that will screw things up unless immediately addressed. It must be someone's law already, but if it isn't I call dibs on it and it should thus be called "Potter's Law":). On a night before launch that I recently experienced, I had a heart stopping moment. The last 5 days of the sprint I had been churning out last minute changes based on usability feedback from the client. In the process, I had neglected to include benchmarking and memory leak testing with the same discipline I prefer. On the night before launch I ran my usual benchmarking scripts. The results were pretty good except on one action. We could have lived with that performance, but the real problem came when I ran tests to detect memory leaks in my Rails application. Let the real fun begin! Before continuing, I should mention there is only thing I hate more than uber visual tasks (editing/creating graphics, layout tweaking, etc.) and that is debugging memory leaks. I ran the ab (Apache Bench) utility on a few different types of pages in the staging environment and didn't notice any problems. The Rails application was consistently teetering under 50M RAM. Excellent! Then I tried the second most requested type of page (this was a static site we were rewriting in Rails to create an easy to use domain-specific CMS - so we had live production web statistics) and my heart skipped a beat or three. Memory usage spiraled out of control. From 49.5M the single mongrel process eventually grew to 250M after a few ab -c2 -n100 .... runs. I first checked out the view since I knew the controller action code was only 6 lines (how much could go wrong there?). I tried removing different parts of the view code and rerunning my tests while monitoring the memory usage of the process. Still no change. So I reluctantly looked in the controller and saw something like the following:


  def show
    @eto = ExchangeTradedOption.find_active(params[:id])

    respond_to do |format|
      format.html # show.rhtml
      format.xml  { render :action => "xml", :layout => false }
      format.csv  { render :action => "csv", :layout => false }
    end
  end
What on earth was in the ExchangeTradedOption.find_active method?

  class << self
    def find_active(id)
      now = Time.now
      find(
        id,
        :include => [:exchange, {:vendor_symbols => [:vendor]}],
        :conditions => [
          %{options.expires_at <= ? AND options.active = 1 AND 
            vendor_symbols.effective_date <= ? AND vendor_symbols.expiration_date >= ?}, 
          now, now, now
        ],
        :order => 'vendor_symbols.effective_date DESC'
      )
    end
  end
Ouch! The problem was that I still needed all vendor symbols and vendors for the view in question and didn't want to make the extra SQL queries for the Vendor on each VendorSymbol as that would have added between 5-10 extra SQL queries per action invocation. I guessed the nested includes in the find was most likely to be causing the issue. So I refactored as followed:

# Controller action code: ExchangeTradedOptionsController#show
  def show
    @eto = ExchangeTradedOption.find_active(params[:id])
    @vendor_symbols = VendorSymbol.find_active_for(params[:id])

    respond_to do |format|
      format.html # show.rhtml
      format.xml  { render :action => "xml", :layout => false }
      format.csv  { render :action => "csv", :layout => false }
    end
  end

# Model finder code: ExchangeTradedOption.find_active
  class << self
    def find_active(id)
      now = Time.now
      find(
        id,
        :include => [:exchange],
        :conditions => [%{options.expires_at <= ? AND options.active = 1}, now],
      )
    end
  end

# The VendorSymbol.find_active_for method implementation is left as an exercise for the reader
# but it is very, very simple!

# Changed view to refer to @vendor_symbols array instead of @eto.vendor_symbols
# This also helped reduce indirection and prevented Law of Demeter violations in the view code!
Of course, I didn't leave it without verifying! Never just guess that the problem is solved, you should always verify this with tests (either automated or manual). There were a few solutions, of course. One is the one I provided above. The second solution I thought of trying was removing the vendor_symbols SQL conditions and sorting in Ruby. This seemed a waste of CPU to me and extra lines of code I felt was unnecessary (remember the less code you write the less you need to maintain!). The third solution was to execute an optimized raw SQL query myself. I also didn't like this option as the first one presented about seemed cleaner and more maintainable going forward. The first solution does cause more than one SQL query to be executed each time the action is run, but it is scalable as it remains constant at two queries per invocation. In addition I added one extra instance variable, which is also not ideal, but again this didn't seem to me to be the worst problem at this stage with only two instance variables in the action being set for the view. The benchmarking results proved my assumptions correct regarding the various solutions. The moral of the story is find the best solution based on the context rather than attempting to apply a one-query-per-action-invocation rule to the whole application, which might be ideal, but occasionally unrealistic and/or catastrophic.