The value of Rails worst practices

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: