Skip to content

Latest commit

 

History

History
1082 lines (854 loc) · 31.7 KB

STYLEGUIDE.md

File metadata and controls

1082 lines (854 loc) · 31.7 KB
Raw

Ruby Style Guide

This is GitHub's Ruby Style Guide, inspired by RuboCop's guide.

Table of Contents

  1. Layout
    1. Indentation
    2. Inline
    3. Newlines
  2. Maximum Line Length
  3. Classes
  4. Collections
  5. Documentation
  6. Dynamic Dispatch
  7. Exceptions
  8. Hashes
  9. Keyword Arguments
  10. Naming
  11. Percent Literals
  12. Regular Expressions
  13. Requires
  14. Strings
  15. Methods
    1. Method definitions
    2. Method calls
  16. Conditional Expressions
    1. Conditional keywords
    2. Ternary operator
  17. Syntax
  18. Rails
    1. content_for
    2. Instance Variables in Views

Layout

Indentation

# bad
message = case
          when song.name == "Misty"
            "Not again!"
          when song.duration > 120
            "Too long!"
          when Time.now.hour > 21
            "It's too late"
          else
            song.to_s
          end

# good
message = case
when song.name == "Misty"
  "Not again!"
when song.duration > 120
  "Too long!"
when Time.now.hour > 21
  "It's too late"
else
  song.to_s
end

# good
case
when song.name == "Misty"
  puts "Not again!"
when song.duration > 120
  puts "Too long!"
when Time.now.hour > 21
  puts "It's too late"
else
  song.play
end

Inline

sum = 1 + 2
a, b = 1, 2
1 > 2 ? true : false; puts "Hi"
[1, 2, 3].each { |e| puts e }
some(arg).other
[1, 2, 3].length
!array.include?(element)

Newlines

def some_method
  data = initialize(options)

  data.manipulate!

  data.result
end

def some_method
  result
end

Maximum Line Length

  • Keep each line of code to a readable length. Unless you have a reason to, keep lines to a maximum of 118 characters. Why 118? That's the width at which the pull request diff UI needs horizontal scrolling (making pull requests harder to review). [link]

Classes

class Parent
  @@class_var = "parent"

  def self.print_class_var
    puts @@class_var
  end
end

class Child < Parent
  @@class_var = "child"
end

Parent.print_class_var # => will print "child"
As you can see all the classes in a class hierarchy actually share one
class variable. Class instance variables should usually be preferred
over class variables.

class TestClass
  # bad
  def TestClass.some_method
    # body omitted
  end

  # good
  def self.some_other_method
    # body omitted
  end
class TestClass
  # bad
  class << self
    def first_method
      # body omitted
    end

    def second_method_etc
      # body omitted
    end
  end

  # good
  class << self
    attr_accessor :per_page
    alias_method :nwo, :find_by_name_with_owner
  end

  def self.first_method
    # body omitted
  end

  def self.second_method_etc
    # body omitted
  end
end
class SomeClass
  def public_method
    # ...
  end

  private
  def private_method
    # ...
  end
end
class SomeClass
  attr_accessor :message

  def greeting(name)
    message = "Hi #{name}" # local variable in Ruby, not attribute writer
    self.message = message
  end
end

Collections

# bad
STATES = ["draft", "open", "closed"]

# good
STATES = %w(draft open closed)

  • Use Set instead of Array when dealing with unique elements. Set implements a collection of unordered values with no duplicates. This is a hybrid of Array's intuitive inter-operation facilities and Hash's fast lookup. [link]

  • Use symbols instead of strings as hash keys. [link]

# bad
hash = { "one" => 1, "two" => 2, "three" => 3 }

# good
hash = { one: 1, two: 2, three: 3 }

Documentation

Use TomDoc to the best of your ability. It's pretty sweet: [link]

# Public: Duplicate some text an arbitrary number of times.
#
# text  - The String to be duplicated.
# count - The Integer number of times to duplicate the text.
#
# Examples
#
#   multiplex("Tom", 4)
#   # => "TomTomTomTom"
#
# Returns the duplicated String.
def multiplex(text, count)
  text * count
end

Dynamic Dispatch

Avoid calling send and its cousins unless you really need it. Metaprogramming can be extremely powerful, but in most cases you can write code that captures your meaning by being explicit: [link]

# avoid
unless [:base, :head].include?(base_or_head)
  raise ArgumentError, "base_or_head must be either :base or :head"
end

repository = pull.send("#{base_or_head}_repository")
branch = pull.send("#{base_or_head}_ref_name")

# prefer
case base_or_head
when :base
  repository = pull.base_repository
  branch = pull.base_ref_name
when :head
  repository = pull.head_repository
  branch = pull.head_ref_name
else
  raise ArgumentError, "base_or_head must be either :base or :head"
end

Exceptions

  • Don't use exceptions for flow of control. [link]
# bad
begin
  n / d
rescue ZeroDivisionError
  puts "Cannot divide by 0!"
end

# good
if d.zero?
  puts "Cannot divide by 0!"
else
  n / d
end
  • Rescue specific exceptions, not StandardError or its superclasses. [link]
# bad
begin
  # an exception occurs here
rescue
  # exception handling
end

# still bad
begin
  # an exception occurs here
rescue Exception
  # exception handling
end

Hashes

Use the Ruby 1.9 syntax for hash literals when all the keys are symbols: [link]

# bad
user = {
  :login => "defunkt",
  :name => "Chris Wanstrath"
}

# good
user = {
  login: "defunkt",
  name: "Chris Wanstrath"
}

Use the 1.9 syntax when calling a method with Hash options arguments or named arguments: [link]

# bad
user = User.create(:login => "jane")
link_to("Account", :controller => "users", :action => "show", :id => user)

# good
user = User.create(login: "jane")
link_to("Account", controller: "users", action: "show", id: user)

If you have a hash with mixed key types, use the legacy hashrocket style to avoid mixing styles within the same hash: [link]

``` ruby
# bad
hsh = {
  user_id: 55,
  "followers-count" => 1000
}

# good
hsh = {
  :user_id => 55,
  "followers-count" => 1000
}

Keyword Arguments

Keyword arguments are recommended but not required when a method's arguments may otherwise be opaque or non-obvious when called. Additionally, prefer them over the old "Hash as pseudo-named args" style from pre-2.0 ruby. [link]

So instead of this:

def remove_member(user, skip_membership_check=false)
  # ...
end

# Elsewhere: what does true mean here?
remove_member(user, true)

Do this, which is much clearer:

def remove_member(user, skip_membership_check: false)
  # ...
end

# Elsewhere, now with more clarity:
remove_member(user, skip_membership_check: true)

Naming

Percent Literals

STATES = %w(draft open closed)
# bad (no interpolation needed)
%(<div class="text">Some text</div>)
# should be "<div class=\"text\">Some text</div>"

# bad (no double-quotes)
%(This is #{quality} style)
# should be "This is #{quality} style"

# bad (multiple lines)
%(<div>\n<span class="big">#{exclamation}</span>\n</div>)
# should be a heredoc.

# good (requires interpolation, has quotes, single line)
%(<tr><td class="name">#{name}</td>)
# bad
%r(\s+)

# still bad
%r(^/(.*)$)
# should be /^\/(.*)$/

# good
%r(^/blog/2011/(.*)$)

Regular Expressions

# bad
/(regexp)/ =~ string
...
process $1

# good
/(?<meaningful_var>regexp)/ =~ string
...
process meaningful_var
  • Be careful with ^ and $ as they match start/end of line, not string endings. If you want to match the whole string use: \A and \z. [link]
string = "some injection\nusername"
string[/^username$/]   # matches
string[/\Ausername\z/] # don't match
  • Use x modifier for complex regexps. This makes them more readable and you can add some useful comments. Just be careful as spaces are ignored. [link]
regexp = %r{
  start         # some text
  \s            # white space char
  (group)       # first group
  (?:alt1|alt2) # some alternation
  end
}x

Requires

Always require dependencies used directly in a script at the start of the same file. Resources that will get autoloaded on first use—such as Rails models, controllers, or helpers—don't need to be required. [link]

require "set"
require "time"

%w(foo bar).to_set
Time.parse("2015-10-21")

This not only loads the necessary dependencies if they haven't already, but acts as documentation about the libraries that the current file uses.

Strings

# bad
email_with_name = user.name + " <" + user.email + ">"

# good
email_with_name = "#{user.name} <#{user.email}>"
  • Use double-quoted strings. Interpolation and escaped characters will always work without a delimiter change, and ' is a lot more common than " in string literals. [link] 
# bad
name = 'Bozhidar'

# good
name = "Bozhidar"
  • Avoid using String#+ when you need to construct large data chunks. Instead, use String#<<. Concatenation mutates the string instance in-place and is always faster than String#+, which creates a bunch of new string objects. [link]
# good and also fast
html = ""
html << "<h1>Page title</h1>"

paragraphs.each do |paragraph|
  html << "<p>#{paragraph}</p>"
end

Methods

Method definitions

def some_method
  # body omitted
end

def some_method_with_arguments(arg1, arg2)
  # body omitted
end

Method calls

# bad
f (3 + 2) + 1

# good
f(3 + 2) + 1

Conditional Expressions

Conditional keywords

# bad
if some_condition then
  # body omitted
end

# good
if some_condition
  # body omitted
end
# bad
if some_condition
  do_something
end

# good
do_something if some_condition
# bad
unless success?
  puts "failure"
else
  puts "success"
end

# good
if success?
  puts "success"
else
  puts "failure"
end
# bad
if (x > 10)
  # body omitted
end

# good
if x > 10
  # body omitted
end

Ternary operator

  • Avoid the ternary operator (?:) except in cases where all expressions are extremely trivial. However, do use the ternary operator(?:) over if/then/else/end constructs for single line conditionals. [link] 
# bad
result = if some_condition then something else something_else end

# good
result = some_condition ? something : something_else
# bad
some_condition ? (nested_condition ? nested_something : nested_something_else) : something_else

# good
if some_condition
  nested_condition ? nested_something : nested_something_else
else
  something_else
end

Syntax

  • Never use for, unless you know exactly why. Most of the time iterators should be used instead. for is implemented in terms of each (so you're adding a level of indirection), but with a twist - for doesn't introduce a new scope (unlike each) and variables defined in its block will be visible outside it. [link] 
arr = [1, 2, 3]

# bad
for elem in arr do
  puts elem
end

# good
arr.each { |elem| puts elem }
  • Prefer {...} over do...end for single-line blocks. Avoid using {...} for multi-line blocks (multiline chaining is always ugly). Always use do...end for "control flow" and "method definitions" (e.g. in Rakefiles and certain DSLs). Avoid do...end when chaining. [link] 
names = ["Bozhidar", "Steve", "Sarah"]

# good
names.each { |name| puts name }

# bad
names.each do |name|
  puts name
end

# good
names.select { |name| name.start_with?("S") }.map { |name| name.upcase }

# bad
names.select do |name|
  name.start_with?("S")
end.map { |name| name.upcase }

  • Some will argue that multiline chaining would look OK with the use of {...}, but they should ask themselves: is this code really readable and can't the block's contents be extracted into nifty methods?

  • Avoid return where not required. [link]

# bad
def some_method(some_arr)
  return some_arr.size
end

# good
def some_method(some_arr)
  some_arr.size
end
# bad
def some_method(arg1=:default, arg2=nil, arg3=[])
  # do something...
end

# good
def some_method(arg1 = :default, arg2 = nil, arg3 = [])
  # do something...
end

While several Ruby books suggest the first style, the second is much more prominent in practice (and arguably a bit more readable).

  • Using the return value of = (an assignment) is ok. [link]
# bad
if (v = array.grep(/foo/)) ...

# good
if v = array.grep(/foo/) ...

# also good - has correct precedence.
if (v = next_value) == "hello" ...
# set name to Bozhidar, only if it's nil or false
name ||= "Bozhidar"
# bad - would set enabled to true even if it was false
enabled ||= true

# good
enabled = true if enabled.nil?
# bad
result = hash.map { |k, v| v + 1 }

# good
result = hash.map { |_, v| v + 1 }

  • Don't use the === (threequals) operator to check types. === is mostly an implementation detail to support Ruby features like case, and it's not commutative. For example, String === "hi" is true and "hi" === String is false. Instead, use is_a? or kind_of? if you must. [link]

    Refactoring is even better. It's worth looking hard at any code that explicitly checks types.

Rails

content_for

Limit usage of content_for helper. The use of content_for is the same as setting an instance variable plus capture.

<% content_for :foo do %>
  Hello
<% end %>

Is effectively the same as

<% @foo_content = capture do %>
  Hello
<% end %>

See "Instance Variables in Views" below.

Common Anti-patterns

Using content_for within the same template to capture data.

Instead, just use capture.

<!-- bad -->
<% content_for :page do %>
  Hello
<% end %>
<% if foo? %>
  <div class="container">
    <%= yield :page %>
  </div>
<% else %>
  <%= yield :page %>
<% end %>

Simply capture and use a local variable since the result is only needed in this template.

<!-- good -->
<% page = capture do %>
  Hello
<% end %>
<% if foo? %>
  <div class="container">
    <%= page %>
  </div>
<% else %>
  <%= page %>
<% end %>

Using content_for to pass content to a subtemplate.

Instead, render layout: with a block.

<!-- bad -->
<% content_for :page do %>
  Hello
<% end %>
<%= render partial: "page" %>
<!-- _page.html.erb -->
<div class="container">
  <%= yield :page %>
</div>

Pass the content in a block directly to the render function.

<!-- good -->
<%= render layout: "page" do %>
  Hello
<% end %>
<!-- _page.html.erb -->
<div class="container">
  <%= yield %>
</div>

Instance Variables in Views

In general, passing data between templates with instance variables is discouraged. This even applies from controllers to templates, not just between partials.

:locals can be used to pass data from a controller just like partials.

def show
  render "blob/show", locals: {
    :repository => current_repository,
    :commit     => current_commit,
    :blob       => current_blob
  }
end

Rails implicit renders are also discouraged.

Always explicitly render templates with a full directory path. This makes template callers easier to trace. You can find all the callers of "app/view/site/hompage.html.erb" with a simple project search for "site/homepage".

def homepage
  render "site/homepage"
end

Exceptions

There are some known edge cases where you might be forced to use instance variables. In these cases, its okay to do so.

Legacy templates

If you need to call a subview that expects an instance variable be set. If possible consider refactoring the subview to accept a local instead.

Layouts

Unfortunately the only way to get data into a layout template is with instance variables. You can't explicitly pass locals to them.