Rails Pull Request Checklist

Many professions have a checklist. Doctors and pilots are both highly skilled jobs that use checklists in order to minimise errors. As developers we have many things to keep in our heads and anything that we can offload to a checklist seems like a win.

Here is a checklist that I use for reviewing pull requests:

Object Oriented Design

  • Methods should be no longer than 5 lines
  • Pass no more than 4 parameters into a method
  • Controllers should¬†instantiate no more than one class
  • Controllers should have a maximum of one instance variable
  • Check that classes have single responsibility. ask yourself: “what does this class do?”. If it parses the response AND sends an email, then it violates single responsibility.
  • Ensure that code adheres to SOLID (Single responsibility, Open-closed, Liskov substitution, Interface segregation and Dependency inversion) principles.

Queries

  • Are you iterating over a collection that will be large in production? In this example, the query will become slow if there are lots of addresses in the table.
    Address.all.each {|a| a.first_line == first_line}

Migrations

  • Migrations should be reversible
  • If migrations cannot be reversed then they should raise raise an error:
    def down
      fail ActiveRecord::IrreversibleMigration
    end
  • Ensure that all columns have sensible defaults

Automated Tools

You should also use automated tools to help you review PRs:

  • Hound CI allows you to agree on a coding style and have it enforced by your CI server
  • Brakeman scans your source code looking for security issues. You can configure your CI server to run brakeman

Credits

This checklist has been influenced by: Sandi Metz, Max Holder