The value of Rails worst practices
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: