Ruby

  • We largely follow the GitHub and bbatsov Ruby style guides. We use RuboCop to enforce style: a violation fails the Continuous Integration build.
  • Don’t use protected when private will do. You almost never need protected. If you, do leave a comment because it is rare enough that others may be confused.
  • Don’t make methods public unless they are actually called by other objects. This includes methods created with syntactical sugar like attr_reader and attr_accessor. This helps communicates to other programmers how the object is intended to be used.
  • Use the Ruby 1.9 hash literal syntax when your hash keys are symbols. Click for more.
  # bad
  hash = { :one => 1, :two => 2, :three => 3 }

  # good
  hash = { one: 1, two: 2, three: 3 }
  • snake_case file names in all ruby projects, including images, javascripts, etc. So image_tag "my_cool_pic.png"
  • Do not use unless with multiple conditions.
  # bad
  unless foo? && bar?

  # good
  if !(foo? && bar?)
  • Use fail when raising new exceptions. Use raise when re-raising an existing exception.
  # bad
  def add_positive(num)
    raise ArgumentError unless num > 0
    ...
  end

  # good
  def add_positive(num)
    fail ArgumentError unless num > 0
    ...
  end

  # bad
  def post(data)
    request(:post, data).status
  rescue NetworkError => e
    if e.status
      e.status
    else
      fail e
    end
  end

  # good
  def post(data)
    request(:post, data).status
  rescue NetworkError => e
    if e.status
      e.status
    else
      raise e
    end
  end
  • Avoid matching on the message String of errors
  # Bad
  def foo(x)
    fail "x must be > 0" unless x > 0
  end

  def bar(x)
    foo(x * 2)
  rescue => e
    if e =~ /must be/
      foo(x * -2)
    else
      raise
    end
  end

  # Better
  def foo(x)
    fail ArgumentError, "x must be > 0" unless x > 0
  end

  def bar(x)
    foo(x * 2)
  rescue ArgumentError => e
    foo(x * -2)
  end

  # Best
  class NegativeArgumentError < ArgumentError
    def initialize(x)
      @x = x
    end
    def message
      "Argument must be > 0, was '#{@x}'"
    end
  end
  def foo(x)
    fail NegativeArgumentError.new(x) unless x > 0
  end

  def bar(x)
    foo(x * 2)
  rescue NegativeArgumentError => e
    foo(x * -2)
  end

URIs

Use the URI library to build up the URIs, not string interpolation

# Do
uri = URI(host)
uri.path = path
uri.query = URI.encode_www_form("q" => "ruby", "lang" => "en")

uri_string = uri.to_s


# Don't
uri_string = host + path + "?" + query

Testing

  • Use Rspec
  • Make sure you write specs against how you think a future developer will break your code.
  • Betterspecs is a good guide for RSpec style, although we are willing to break the single expectation rule to be pragmatic.
  • Don’t start describe blocks with conjunctions.
  # bad
  describe "when using invalid credentials" do

  # good
  describe "using invalid credentials" do
  • Use not_to rather than to_not.

Acceptance tests

  • Be conscious of asynchronous test drivers (capybara-webkit, poltergeist, etc.). Otherwise, you will get random test failures.
   # Bad. May randomly fail.
   click_link "Go somewhere"
   expect(page.current_path).to eql("/some/path")

   # Good. Will wait for the path to update as the request is processed.
   click_link "Go somewhere"
   expect(page).to have_current_path("/some/path")

Rails

  • Follow the Rails community style guide
  • Variables should never be set in views.
    • Use a helper method, set an instance variable in the controller, or render different views for each option.
  • Avoid alias_method_chain (or similar) when super will do.
  • When extracting a concern from a fat model
  • Try to use the DHH philosophy for controllers: only use the default CRUD actions.
  • Give new endpoints the “obscenely-large-account test”: Imagine what happens when it is used by our largest customers. Use background jobs with the 202 Accepted pattern when appropriate.

Migrations

  • Before deploying a migration, request a code review from someone who has made a lot of migration mistakes.
  • Since we have customers using our site every second, all migrations should be zero-downtime. We follow the patterns outlined here: Rails Migrations with Zero Downtime
  • If the DB reference indicates your migration operations will be expensive, you will need to use LHM to be zero-downtime.
  • Avoid using migrations to modify data. Use a (temporary) rake task to modify the data, and save the migration for changes to the database schema. This keeps the migrations easy to read. It also helps keep the ever changing model classes out of the migration. Finally, it forces you to acknowledge that migrations involving data changes are generally not atomic.
  • Boolean columns should be null: false so we can run queries like where(collapsed: false) without worrying about null values.

Concerns

  • Don’t use ActiveSupport::Concern. Do use Module.included. These modules may still live in the ./app/*/concerns folder.
  • Isolate as much of the concern as possible into a class. Check out the commmit on Github as an example. Notice how its easier to isolate the validation tests to the class.

Handling exceptions we don’t plan on fixing

There are a number of exceptions that get raised but not worth the time to fix. We want to leave them out of Airbrake so we’ll only be notified about the exceptions that are truly exceptional. But we don’t want to completely hide these errors in case they get more common.

We want to send these exceptions to NewRelic, where they will appear in the “[Events | Alerts]” tab. NewRelic does not notify us of exceptions unless our error rate goes above a certain threshold.

Options:

  • Send it off to NewRelic and handle the error yourself:
rescue UnimportantException => e
  NewRelic::Agent.notice_error(e)
  render json: e.message, status: :unprocessable_entity
end
  • Reraise as an exception that is already excluded from Airbrake:
rescue StandardError => e
  raise(AlreadyIgnoredException.new(e))
end
  • Exclude the exception from Airbrake:

Warning: Be careful with this approach. You should be confident that you aren’t ignoring exceptions from other places that you aren’t aware of.

# config/initializers/airbrake.rb
config.ignore << "UnimportantException"