How to Skim a Pull Request

We review quite a few code diffs; it is our job. Often, when jumping on a new project, we start by reviewing pull requests.

After years of this we have each developed a few tricks to find problematic areas when reviewing our first pull request on a legacy codebase. And so I present to you an unexplained, incomplete, and arbitrarily grouped list of keywords that will cause us to read your Rails code more with more care and suspicion.

None of these are wrong, but all of them are noteworthy.

The times, they are a-changin’

• Calling ActiveRecord::Base#create, ActiveRecord::Base#save, or ActiveRecord::Base#update_attributes without checking the return value,
• #map!, #select!, #reject!,
• Hash#[]=, Array#[]=,
• if without an else,
• #try,
• while, and
• assigning to an instance variable outside of initialize (or #perform).

They walked in line

• return,
• #each,
• #let!,
• subject,
• before_filter, and
• acts_as_tenant.

We are the world

• .find_or_create_by,
• app/*/concerns/*.rb,
• has_one, and
• DISTINCT in a SQL selector.

Generals and majors

• A state column, and especially any state machine gem,
• bare strings,
• ENV#[],
• JSON.parse,
• unless,
• rand,
• protected, and
• the eq matcher.