Rails

#bad
Book.update_attributes!(author: 'Alice')

#good
Book.update!(author: 'Alice')

# bad
class Person < ApplicationRecord
  after_commit :after_commit_callback
  before_validation :before_validation_callback
end

# good
class Person < ApplicationRecord
  before_validation :before_validation_callback
  after_commit :after_commit_callback
end

# bad
class Book < ApplicationRecord
  def save
    self.title = title.upcase!
    super
  end
end

# good
class Book < ApplicationRecord
  before_save :upcase_title

  def upcase_title
    self.title = title.upcase!
  end
end

# good
'some_string'.start_with?('prefix')
'some_string'.end_with?('suffix')
[1, 2, 'a'] << 'b'
[1, 2, 'a'].unshift('b')

# bad
'some_string'.starts_with?('prefix')
'some_string'.ends_with?('suffix')
[1, 2, 'a'].append('b')
[1, 2, 'a'].prepend('b')

# good
class MyController < ApplicationController
  # ...
end

# bad
class MyController < ActionController::Base
  # ...
end

# good
class Rails5Job < ApplicationJob
  # ...
end

# bad
class Rails4Job < ActiveJob::Base
  # ...
end

# good
class MyMailer < ApplicationMailer
  # ...
end

# bad
class MyMailer < ActionMailer::Base
  # ...
end

# good
class Rails5Model < ApplicationRecord
  # ...
end

# bad
class Rails4Model < ActiveRecord::Base
  # ...
end

# bad
assert !x

# good
assert_not x

# bad
class Post < ApplicationRecord
  belongs_to :blog, required: false
end

# good
class Post < ApplicationRecord
  belongs_to :blog, optional: true
end

# bad
class Post < ApplicationRecord
  belongs_to :blog, required: true
end

# good
class Post < ApplicationRecord
  belongs_to :blog, optional: false
end

# bad
def change
  add_column :users, :name, :string, null: false
  add_column :users, :nickname, :string

  # ALTER TABLE `users` ADD `name` varchar(255) NOT NULL
  # ALTER TABLE `users` ADD `nickname` varchar(255)
end

# good
def change
  change_table :users, bulk: true do |t|
    t.string :name, null: false
    t.string :nickname
  end

  # ALTER TABLE `users` ADD `name` varchar(255) NOT NULL,
  #                     ADD `nickname` varchar(255)
end

# bad
def change
  change_table :users do |t|
    t.string :name, null: false
    t.string :nickname
  end
end

# good
def change
  change_table :users, bulk: true do |t|
    t.string :name, null: false
    t.string :nickname
  end
end

# good
# When you don't want to combine alter queries.
def change
  change_table :users, bulk: false do |t|
    t.string :name, null: false
    t.string :nickname
  end
end

# bad
content_tag(:p, 'Hello world!')
content_tag(:br)

# good
tag.p('Hello world!')
tag.br
content_tag(name, 'Hello world!')

# bad
create_table :users

# bad
create_table :users do |t|
  t.string :name
  t.string :email
end

# good
create_table :users do |t|
  t.string :name
  t.string :email

  t.timestamps
end

# good
create_table :users do |t|
  t.string :name
  t.string :email

  t.datetime :created_at, default: -> { 'CURRENT_TIMESTAMP' }
end

# good
create_table :users do |t|
  t.string :name
  t.string :email

  t.datetime :updated_at, default: -> { 'CURRENT_TIMESTAMP' }
end

# bad
default_scope -> { where(hidden: false) }

# good
scope :published, -> { where(hidden: false) }

# bad
def self.default_scope
  where(hidden: false)
end

# good
def self.published
  where(hidden: false)
end

# bad
def bar
  foo.bar
end

# good
delegate :bar, to: :foo

# good
def bar
  foo&.bar
end

# good
private
def bar
  foo.bar
end

# bad
delegate :foo, to: :bar, allow_blank: true

# good
delegate :foo, to: :bar, allow_nil: true

# bad
User.find_by_name(name)
User.find_by_name_and_email(name)
User.find_by_email!(name)

# good
User.find_by(name: name)
User.find_by(name: name, email: email)
User.find_by!(email: email)

# bad
enum status: [:active, :archived]

# good
enum status: { active: 0, archived: 1 }

# bad
enum status: { active: 0, archived: 0 }

# good
enum status: { active: 0, archived: 1 }

# bad
enum status: [:active, :archived, :active]

# good
enum status: [:active, :archived]

# bad
Rails.env == 'production'

# bad, always returns false
Rails.env == :test

# good
Rails.env.production?

# bad
exit(0)

# good
raise 'a bad error has happened'

# bad
User.where(name: 'Bruce').first
User.where(name: 'Bruce').take

# good
User.find_by(name: 'Bruce')

# bad
User.where(id: id).take!
User.find_by_id!(id)
User.find_by!(id: id)

# good
User.find(id)

# bad
User.all.each

# good
User.all.find_each

# bad
# has_and_belongs_to_many :ingredients

# good
# has_many :ingredients, through: :recipe_ingredients

# bad
class User < ActiveRecord::Base
  has_many :comments
  has_one :avatar
end

# good
class User < ActiveRecord::Base
  has_many :comments, dependent: :restrict_with_exception
  has_one :avatar, dependent: :destroy
  has_many :patients, through: :appointments
end

# bad
def welcome_message
  "Hello #{@user.name}"
end

# good
def welcome_message(user)
  "Hello #{user.name}"
end

# bad
get :new, { user_id: 1}

# good
get :new, params: { user_id: 1 }

# bad
class MyPageController < ApplicationController
  skip_before_action :login_required,
    only: :show, if: :trusted_origin?
end

# good
class MyPageController < ApplicationController
  skip_before_action :login_required,
    if: -> { trusted_origin? && action_name == "show" }
end

# bad
class MyPageController < ApplicationController
  skip_before_action :login_required,
    except: :admin, if: :trusted_origin?
end

# good
class MyPageController < ApplicationController
  skip_before_action :login_required,
    if: -> { trusted_origin? && action_name != "admin" }
end

# bad
[1, 2, 3].each_with_object({}) { |el, h| h[foo(el)] = el }
[1, 2, 3].map { |el| [foo(el), el] }.to_h
Hash[[1, 2, 3].collect { |el| [foo(el), el] }]

# good
[1, 2, 3].index_by { |el| foo(el) }

# bad
[1, 2, 3].each_with_object({}) { |el, h| h[el] = foo(el) }
[1, 2, 3].map { |el| [el, foo(el)] }.to_h
Hash[[1, 2, 3].collect { |el| [el, foo(el)] }]

# good
[1, 2, 3].index_with { |el| foo(el) }

# bad - String#inquiry
ruby = 'two'.inquiry
ruby.two?

# good
ruby = 'two'
ruby == 'two'

# bad - Array#inquiry
pets = %w(cat dog).inquiry
pets.gopher?

# good
pets = %w(cat dog)
pets.include? 'cat'

# good
class Blog < ApplicationRecord
  has_many :posts
end

class Post < ApplicationRecord
  belongs_to :blog
end

# bad
class Blog < ApplicationRecord
  has_many :posts, -> { order(published_at: :desc) }
end

class Post < ApplicationRecord
  belongs_to :blog
end

# good
class Blog < ApplicationRecord
  has_many(:posts,
           -> { order(published_at: :desc) },
           inverse_of: :blog)
end

class Post < ApplicationRecord
  belongs_to :blog
end

# good
class Blog < ApplicationRecord
  with_options inverse_of: :blog do
    has_many :posts, -> { order(published_at: :desc) }
  end
end

class Post < ApplicationRecord
  belongs_to :blog
end

# good
# When you don't want to use the inverse association.
class Blog < ApplicationRecord
  has_many(:posts,
           -> { order(published_at: :desc) },
           inverse_of: false)
end

# bad
class Picture < ApplicationRecord
  belongs_to :imageable, polymorphic: true
end

class Employee < ApplicationRecord
  has_many :pictures, as: :imageable
end

class Product < ApplicationRecord
  has_many :pictures, as: :imageable
end

# good
class Picture < ApplicationRecord
  belongs_to :imageable, polymorphic: true
end

class Employee < ApplicationRecord
  has_many :pictures, as: :imageable, inverse_of: :imageable
end

class Product < ApplicationRecord
  has_many :pictures, as: :imageable, inverse_of: :imageable
end

# bad
# However, RuboCop can not detect this pattern...
class Physician < ApplicationRecord
  has_many :appointments
  has_many :patients, through: :appointments
end

class Appointment < ApplicationRecord
  belongs_to :physician
  belongs_to :patient
end

class Patient < ApplicationRecord
  has_many :appointments
  has_many :physicians, through: :appointments
end

# good
class Physician < ApplicationRecord
  has_many :appointments
  has_many :patients, through: :appointments
end

class Appointment < ApplicationRecord
  belongs_to :physician, inverse_of: :appointments
  belongs_to :patient, inverse_of: :appointments
end

class Patient < ApplicationRecord
  has_many :appointments
  has_many :physicians, through: :appointments
end

# bad
class LoginController < ApplicationController
  before_action :require_login, only: %i[index settings logout]

  def index
  end
end

# good
class LoginController < ApplicationController
  before_action :require_login, only: %i[index settings logout]

  def index
  end

  def settings
  end

  def logout
  end
end

# bad
module FooMixin
  extend ActiveSupport::Concern

  included do
    before_action proc { authenticate }, only: :foo
  end
end

# good
module FooMixin
  extend ActiveSupport::Concern

  included do
    before_action proc { authenticate }, only: :foo
  end

  def foo
    # something
  end
end

class ContentController < ApplicationController
  def update
    @content.update(content_attributes)
  end
end

class ArticlesController < ContentController
  before_action :load_article, only: [:update]

  # the cop requires this method, but it relies on behaviour defined
  # in the superclass, so needs to invoke `super`
  def update
    super
  end

  private

  def load_article
    @content = Article.find(params[:article_id])
  end
end

# bad
link_to 'Click here', url, target: '_blank'

# good
link_to 'Click here', url, target: '_blank', rel: 'noopener'

# good
link_to 'Click here', url, target: '_blank', rel: 'noreferrer'

# bad
class User < ActionMailer::Base
end

class User < ApplicationMailer
end

# good
class UserMailer < ActionMailer::Base
end

class UserMailer < ApplicationMailer
end

# bad
match ':controller/:action/:id'
match 'photos/:id', to: 'photos#show', via: :get

# good
get ':controller/:action/:id'
get 'photos/:id', to: 'photos#show'
match 'photos/:id', to: 'photos#show', via: [:get, :post]
match 'photos/:id', to: 'photos#show', via: :all

# bad
!array.include?(2)
!hash.include?(:key)

# good
array.exclude?(2)
hash.exclude?(:key)

# bad
add_column :users, :name, :string, null: false
add_reference :products, :category, null: false

# good
add_column :users, :name, :string, null: true
add_column :users, :name, :string, null: false, default: ''
add_reference :products, :category
add_reference :products, :category, null: false, default: 1

# bad
puts 'A debug message'
pp 'A debug message'
print 'A debug message'

# good
Rails.logger.debug 'A debug message'

user_content = "<b>hi</b>"

# bad
"<p>#{user_content}</p>".html_safe
# => ActiveSupport::SafeBuffer "<p><b>hi</b></p>"

# good
content_tag(:p, user_content)
# => ActiveSupport::SafeBuffer "<p>&lt;b&gt;hi&lt;/b&gt;</p>"

# bad
out = ""
out << "<li>#{user_content}</li>"
out << "<li>#{user_content}</li>"
out.html_safe
# => ActiveSupport::SafeBuffer "<li><b>hi</b></li><li><b>hi</b></li>"

# good
out = []
out << content_tag(:li, user_content)
out << content_tag(:li, user_content)
safe_join(out)
# => ActiveSupport::SafeBuffer
#    "<li>&lt;b&gt;hi&lt;/b&gt;</li><li>&lt;b&gt;hi&lt;/b&gt;</li>"

# bad
out = "<h1>trusted content</h1>".html_safe
out.safe_concat(user_content)
# => ActiveSupport::SafeBuffer "<h1>trusted_content</h1><b>hi</b>"

# good
out = "<h1>trusted content</h1>".html_safe
out.concat(user_content)
# => ActiveSupport::SafeBuffer
#    "<h1>trusted_content</h1>&lt;b&gt;hi&lt;/b&gt;"

# safe, though maybe not good style
out = "trusted content"
result = out.concat(user_content)
# => String "trusted content<b>hi</b>"
# because when rendered in ERB the String will be escaped:
# <%= result %>
# => trusted content&lt;b&gt;hi&lt;/b&gt;

# bad
(user_content + " " + content_tag(:span, user_content)).html_safe
# => ActiveSupport::SafeBuffer "<b>hi</b> <span><b>hi</b></span>"

# good
safe_join([user_content, " ", content_tag(:span, user_content)])
# => ActiveSupport::SafeBuffer
#    "&lt;b&gt;hi&lt;/b&gt; <span>&lt;b&gt;hi&lt;/b&gt;</span>"

# bad
Model.pluck(:a).first
[{ a: :b, c: :d }].pluck(:a, :b).first

# good
Model.pick(:a)
[{ a: :b, c: :d }].pick(:a, :b)

# bad
Post.published.map { |post| post[:title] }
[{ a: :b, c: :d }].collect { |el| el[:a] }

# good
Post.published.pluck(:title)
[{ a: :b, c: :d }].pluck(:a)

# bad
User.pluck(:id)
user.posts.pluck(:id)

def self.user_ids
  pluck(primary_key)
end

# good
User.ids
user.posts.ids

def self.user_ids
  ids
end

# bad
Post.where(user_id: User.active.pluck(:id))

# good
Post.where(user_id: User.active.select(:id))

# bad
3.day.ago
1.months.ago

# good
3.days.ago
1.month.ago

# bad
a.present? ? a : nil

# bad
!a.present? ? nil : a

# bad
a.blank? ? nil : a

# bad
!a.blank? ? a : nil

# good
a.presence

# bad
a.present? ? a : b

# bad
!a.present? ? b : a

# bad
a.blank? ? b : a

# bad
!a.blank? ? a : b

# good
a.presence || b

# bad
task :foo do
  do_something
end

# good
task foo: :environment do
  do_something
end

# bad
x = read_attribute(:attr)
write_attribute(:attr, val)

# good
x = self[:attr]
self[:attr] = val

# bad
validates :x, length: { is: 5 }, allow_nil: true, allow_blank: true

# bad
validates :x, length: { is: 5 }, allow_nil: false, allow_blank: true

# bad
validates :x, length: { is: 5 }, allow_nil: false, allow_blank: false

# good
validates :x, length: { is: 5 }, allow_blank: true

# good
validates :x, length: { is: 5 }, allow_blank: false

# good
# Here, `nil` is valid but `''` is not
validates :x, length: { is: 5 }, allow_nil: true, allow_blank: false

# bad
class Post
  has_many :comments, foreign_key: 'post_id'
end

class Comment
  belongs_to :post, foreign_key: 'post_id'
end

# good
class Post
  has_many :comments
end

class Comment
  belongs_to :author, foreign_key: 'user_id'
end

# bad
class Account < ApplicationRecord
  with_options dependent: :destroy do |assoc|
    assoc.has_many :customers
    assoc.has_many :products
    assoc.has_many :invoices
    assoc.has_many :expenses
  end
end

# good
class Account < ApplicationRecord
  with_options dependent: :destroy do
    has_many :customers
    has_many :products
    has_many :invoices
    has_many :expenses
  end
end

# bad
with_options options: false do |merger|
  merger.invoke(merger.something)
end

# good
with_options options: false do
  invoke(something)
end

# good
client = Client.new
with_options options: false do |merger|
  client.invoke(merger.something, something)
end

# ok
# When `with_options` includes a block, all scoping scenarios
# cannot be evaluated. Thus, it is ok to include the explicit
# receiver.
with_options options: false do |merger|
  merger.invoke
  with_another_method do |another_receiver|
    merger.invoke(another_receiver)
  end
end

# bad
has_many :accounts, class_name: Account
has_many :accounts, class_name: Account.name

# good
has_many :accounts, class_name: 'Account'

# bad
class SomeClass
  EXPIRED_AT = 1.week.since
end

# good
class SomeClass
  EXPIRES = 1.week

  def self.expired_at
    EXPIRES.since
  end
end

# good
class SomeClass
  def self.expired_at
    1.week.since
  end
end

# bad
class ProductsController < ApplicationController
  def index
    render inline: "<% products.each do |p| %><p><%= p.name %></p><% end %>", type: :erb
  end
end

# good
# app/views/products/index.html.erb
# <% products.each do |p| %>
#   <p><%= p.name %></p>
# <% end %>

class ProductsController < ApplicationController
  def index
  end
end

# bad - explicit MIME type to `text/plain`
render text: 'Ruby!', content_type: 'text/plain'

# good - short and precise
render plain: 'Ruby!'

# good - explicit MIME type not to `text/plain`
render text: 'Ruby!', content_type: 'text/html'

# bad
def change
  change_table :users do |t|
    t.remove :name
  end
end

# good
def change
  create_table :users do |t|
    t.string :name
  end
end

# good
def change
  reversible do |dir|
    change_table :users do |t|
      dir.up do
        t.column :name, :string
      end

      dir.down do
        t.remove :name
      end
    end
  end
end

# drop_table

# bad
def change
  drop_table :users
end

# good
def change
  drop_table :users do |t|
    t.string :name
  end
end

# change_column_default

# bad
def change
  change_column_default(:suppliers, :qualification, 'new')
end

# good
def change
  change_column_default(:posts, :state, from: nil, to: "draft")
end

# remove_column

# bad
def change
  remove_column(:suppliers, :qualification)
end

# good
def change
  remove_column(:suppliers, :qualification, :string)
end

# remove_foreign_key

# bad
def change
  remove_foreign_key :accounts, column: :owner_id
end

# good
def change
  remove_foreign_key :accounts, :branches
end

# good
def change
  remove_foreign_key :accounts, to_table: :branches
end

# change_table

# bad
def change
  change_table :users do |t|
    t.remove :name
    t.change_default :authorized, 1
    t.change :price, :string
  end
end

# good
def change
  change_table :users do |t|
    t.string :name
  end
end

# good
def change
  reversible do |dir|
    change_table :users do |t|
      dir.up do
        t.change :price, :string
      end

      dir.down do
        t.change :price, :integer
      end
    end
  end
end

# bad
do_something if foo&.blank?
do_something unless foo&.blank?

# good
do_something if foo.blank?
do_something unless foo.blank?

# bad
user.save
user.update(name: 'Joe')
user.find_or_create_by(name: 'Joe')
user.destroy

# good
unless user.save
  # ...
end
user.save!
user.update!(name: 'Joe')
user.find_or_create_by!(name: 'Joe')
user.destroy!

user = User.find_or_create_by(name: 'Joe')
unless user.persisted?
  # ...
end

def save_user
  return user.save
end

# bad
scope :something, where(something: true)

# good
scope :something, -> { where(something: true) }

# bad
I18n.translate :key
I18n.localize Time.now

# good
I18n.t :key
I18n.l Time.now

# bad
Article.first.decrement!(:view_count)
DiscussionBoard.decrement_counter(:post_count, 5)
Article.first.increment!(:view_count)
DiscussionBoard.increment_counter(:post_count, 5)
person.toggle :active
product.touch
Billing.update_all("category = 'authorized', author = 'David'")
user.update_attribute(:website, 'example.com')
user.update_columns(last_request_at: Time.current)
Post.update_counters 5, comment_count: -1, action_count: 1

# good
user.update(website: 'example.com')
FileUtils.touch('file')

# bad - if the schema does not have a unique index
validates :account, uniqueness: true

# good - if the schema has a unique index
validates :account, uniqueness: true

# good - even if the schema does not have a unique index
validates :account, length: { minimum: MIN_LENGTH }

# bad
Rails.env.proudction?
Rails.env == 'proudction'

# good
Rails.env.production?
Rails.env == 'production'

# bad
validates_acceptance_of :foo
validates_confirmation_of :foo
validates_exclusion_of :foo
validates_format_of :foo
validates_inclusion_of :foo
validates_length_of :foo
validates_numericality_of :foo
validates_presence_of :foo
validates_absence_of :foo
validates_size_of :foo
validates_uniqueness_of :foo

# good
validates :foo, acceptance: true
validates :foo, confirmation: true
validates :foo, exclusion: true
validates :foo, format: true
validates :foo, inclusion: true
validates :foo, length: true
validates :foo, numericality: true
validates :foo, presence: true
validates :foo, absence: true
validates :foo, size: true
validates :foo, uniqueness: true

# bad
User.where(name: 'john').exists?
User.where(['name = ?', 'john']).exists?
User.where('name = ?', 'john').exists?
user.posts.where(published: true).exists?

# good
User.exists?(name: 'john')
User.where('length(name) > 10').exists?
user.posts.exists?(published: true)