February 02, 2014 • Permalink • GitHub

Most of the discussion of programming as it relates to learning and assessment revolves around best practices or the right way to do things. However, worst practices have value, too. And when I say "worst practices", I'm thinking about any of the following:

anti-patterns

code smells

bugs

style problems

etc.

Any beginning Rails developer can synthesize Rubydocs, Railscasts, and Stack Overflow posts into a functioning application, but as that application grows in scope and complexity, worst practices will start to creep in. A more experienced developer, however, can draw on years of legacy code maintenance and messy refactoring projects to keep a codebase in check.

When interviewing potential Rails developers, I've found that the quickest way to gauge the experience of a potential hire is to show them some shockingly bad Rails code and ask them what they see. This process takes as long as whiteboarding FizzBuzz while being vastly more revealing. Plus, it can be easily done over the phone.

My entry for a World's Worst Rails Model competition

What follows is an example of a Rails model that should make any good developer cringe. You can hardly count the infelicities. The problems below range from serious bugs to minor stylistic issues and include mistakes I've made myself.

You can hover over (mobile users: click on) highlighted line numbers for annotations describing these mistakes, or check out the GitHub repo for all the code and annotations in this post.

1 class User < ActiveRecord :: Base 2 3 MAILCHIMP_API_KEY = 'm23lm092m3' 4 5 has_many :orders 6 has_many :packages , through : :orders 7 8 before_save :assign_referral_code , on : :create 9 after_create :schedule_welcome_email 10 11 validates_presence_of :email , :fname , :lname 12 validates :referral_code , uniqueness : true 13 14 scope :recently_created , where ( "created_at <= #{ Date . today - 2 . days } " ) 15 16 def name 17 self . fname + ' ' + self . lname 18 end 19 20 def formatted_name 21 "<strong> #{ name } </strong> (<a href= \" mailto: #{ email } \" > #{ email } </a>)" 22 end 23 24 def assign_referral_code 25 referral_code = SecureRandom . hex ( 6 ) 26 end 27 28 def schedule_welcome_email 29 Mailer . delay . welcome ( self ) # Queue email with DelayedJob 30 end 31 32 def has_orders 33 orders . any? 34 end 35 36 def most_recent_package_shipping_address 37 order . last . package . shipping_address 38 end 39 40 def can_manage? 41 ( email = 'manager@example.com' ) or ( email = 'admin@example.com' ) 42 end 43 44 def order_product ( product ) 45 result = OrderProcessor . charge ( self , product ) 46 47 if result 48 Event . log_order_processing ( self ) 49 Mailer . order_confirmation ( self , product ) . deliver 50 return true 51 else 52 Event . log_failed_order_processing ( self ) 53 return false 54 end 55 end 56 57 def self . delete_user ( email ) 58 begin 59 user = User . find_by_email ( email ) 60 user . destroy! 61 rescue Exception => e # email not found 62 Rails . logger . error ( "Could not delete user with email #{ email } " ) 63 end 64 end 65 66 end 67 68 # == Schema Information 69 # 70 # Table name: users 71 # 72 # id :integer not null, primary key 73 # email :string(255) default(""), not null 74 # fname :string(255) default(""), not null 75 # lname :string(255) default(""), not null 76 # referral_code :string(255) 77 # created_at :datetime 78 # updated_at :datetime

Had enough?

Let's have some more fun and do the same for RSpec. Ignore the most obvious issue of poor test coverage and focus on some common violations of style and convention.

1 require 'spec_helper' 2 3 describe User do 4 it 'I can create a new user' do 5 user = User . create ( email : 'test@example.com' , fname : 'John' , lname : 'Doe' ) 6 expect ( user . id ) . to_not be_nil 7 end 8 9 it 'validates the required fields' do 10 user = User . new ( email : nil , fname : nil , lname : nil ) 11 user . errors [ :email ]. should include ( 'is required' ) 12 user . errors [ :fname ]. should include ( 'is required' ) 13 user . errors [ :lname ]. should include ( 'is required' ) 14 end 15 16 it 'assigns a unique referral code' do 17 user = User . create! ( email : 'test@example.com' , fname : 'John' , lname : 'Doe' ) 18 expect ( user . referral_code ) . to_not be_nil 19 end 20 21 # it 'sends a welcome email' do 22 # user = User.create(email: 'test@example.com', fname: 'John', lname: 'Doe') 23 # expect(Delayed::Job.count).to eq(1) 24 # end 25 26 context 'orders' do 27 before ( :all ) do 28 @user = User . create ( email : 'test@example.com' , fname : 'John' , lname : 'Doe' ) 29 @product = Product . create ( name : 'Rails for Dummies' ) 30 end 31 32 before ( :each ) do 33 @orders = [] 34 @packages = [] 35 36 10 . times do 37 order = Order . create ( user : @user , @product : @product ) 38 package = Package . create ( order : order , shipping_address : '123 Street Ave, New York, NY' ) 39 @orders << order 40 @packages << package 41 end 42 end 43 44 it 'returns orders and packages for a user' do 45 expect ( @user . orders . size ) . to eq ( 10 ) 46 expect ( @user . packages . size ) . to eq ( 10 ) 47 end 48 49 context '#most_recent_package' do 50 it 'returns the most recent package' do 51 expect ( @user . most_recent_package ) . to eq ( @packages . last ) 52 end 53 54 it 'the most recent package belongs to the last order' do 55 expect ( @user . most_recent_package . order ) . to eq ( @orders . last ) 56 end 57 end 58 59 it 'the has_orders method indicates whether a user has any orders' do 60 expect ( @user . has_orders ) . to eq ( true ) 61 end 62 end 63 end

Resources

Especially when it comes to coding style, there will be disagreements on the right approach. So, you might disagree with some of the above annotations. For the most part, though, I recommend sticking to these guides: