---
title: How to Skim a Pull Request
teaser: A list of keywords that give us pause.
tags: code,ruby
author: Mike Burns
published_on: 2017-02-13
---

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`][nil],
- `#try`,
- `while`, and
- assigning to an instance variable [outside of `initialize`][immutability] (or `#perform`).

## They walked in line

- [`return`][oo-is-fp],
- [`#each`][iteration],
- [`#let!`][sequencing],
- [`subject`][mystery-guest],
- `before_filter`, and
- `acts_as_tenant`.

## We are the world

- [`.find_or_create_by`][find-or-create],
- [`app/*/concerns/*.rb`][inheritance],
- [`has_one`][1-to-1], and
- [`DISTINCT`][distinct] in a SQL selector.

## Generals and majors

- A `state` column, and especially any state machine gem,
- [bare] strings,
- [`ENV#[]`][fetch],
- [`JSON.parse`][json-schema],
- `unless`,
- [`rand`][random],
- `protected`, and
- the [`eq` matcher][eq].

[bare]: http://guides.rubyonrails.org/i18n.html
[checking the return value]: http://rails-bestpractices.com/posts/2012/11/02/check-the-return-value-of-save-otherwise-use-save/
[distinct]: https://www.red-gate.com/simple-talk/databases/sql-server/t-sql-programming-sql-server/dont-use-distinct-as-a-join-fixer/
[eq]: https://objectpartners.com/2013/09/18/the-benefits-of-using-assertthat-over-other-assert-methods-in-unit-tests/
[fetch]: https://ruby-doc.org/core-2.3.3/ENV.html#method-c-fetch
[find-or-create]: http://api.rubyonrails.org/classes/ActiveRecord/Relation.html#method-i-find_or_create_by
[immutability]: http://www.yegor256.com/2014/06/09/objects-should-be-immutable.html
[inheritance]: https://thoughtbot.com/blog/reusable-oo-composition-vs-inheritance
[1-to-1]: https://stackoverflow.com/a/9688442
[it is our job]: https://github.com/thoughtbot/guides/tree/master/code-review
[iteration]: https://thoughtbot.com/blog/iteration-as-an-anti-pattern
[json-schema]: https://github.com/ruby-json-schema/json-schema/
[mystery-guest]: https://thoughtbot.com/blog/mystery-guest
[nil]: https://thoughtbot.com/blog/if-you-gaze-into-nil-nil-gazes-also-into-you
[oo-is-fp]: http://www.ccs.neu.edu/home/matthias/Presentations/ecoop2004.pdf
[random]: https://johnthedebs.blogspot.com/2008/01/arbitrary-vs-random.html
[review]: https://www.youtube.com/watch?v=PJjmw9TRB7s
[reviewing pull requests]: https://thoughtbot.com/upcase/videos/tips-for-code-review
[sequencing]: http://zacharyvoase.com/2014/04/30/monads/