Rails

# bad
class HomeController < ApplicationController
  def create
    flash[:alert] = "msg"
    render :index
  end
end

# good
class HomeController < ApplicationController
  def create
    flash.now[:alert] = "msg"
    render :index
  end
end

# bad
class MyControllerTest < ActionController::TestCase
end

# good
class MyControllerTest < ActionDispatch::IntegrationTest
end

# bad
def index; end
def destroy; end
def show; end

# good
def index; end
def show; end
def destroy; end

# 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')

# bad
ActiveRecord::Base.include(MyClass)

# good
ActiveSupport.on_load(:active_record) { include MyClass }

# bad (will not add an index)
add_column :table, :column, :integer, index: true

# good
add_column :table, :column, :integer
add_index :table, :column

# bad
# This won't be triggered.
after_create_commit :log_action

# This will override the callback added by
# after_create_commit.
after_update_commit :log_action

# bad
# This won't be triggered.
after_commit :log_action, on: :create
# This won't be triggered.
after_update_commit :log_action
# This will override both previous callbacks.
after_commit :log_action, on: :destroy

# good
after_save_commit :log_action

# good
after_create_commit :log_create_action
after_update_commit :log_update_action

# 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
MyTable.arel_table["*"]

# good
MyTable.arel_table[Arel.star]

# bad
assert !x

# good
assert_not x

# bad
class User < ApplicationRecord
  attribute :confirmed_at, :datetime, default: Time.zone.now
end

# good
class User < ApplicationRecord
  attribute :confirmed_at, :datetime, default: -> { Time.zone.now }
end

# bad
class User < ApplicationRecord
  attribute :roles, :string, array: true, default: []
end

# good
class User < ApplicationRecord
  attribute :roles, :string, array: true, default: -> { [] }
end

# bad
class User < ApplicationRecord
  attribute :configuration, default: {}
end

# good
class User < ApplicationRecord
  attribute :configuration, default: -> { {} }
end

# good
class User < ApplicationRecord
  attribute :role, :string, default: :customer
end

# good
class User < ApplicationRecord
  attribute :activated, :boolean, default: false
end

# good
class User < ApplicationRecord
  attribute :login_count, :integer, default: 0
end

# good
class User < ApplicationRecord
  FOO = 123
  attribute :custom_attribute, :integer, default: FOO
end

# 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
collection.reject(&:blank?)
collection.reject { |_k, v| v.blank? }

# good
collection.compact_blank

# bad
collection.delete_if(&:blank?)            # Same behavior as `Array#compact_blank!` and `Hash#compact_blank!`
collection.delete_if { |_k, v| v.blank? } # Same behavior as `Array#compact_blank!` and `Hash#compact_blank!`
collection.reject!(&:blank?)              # Same behavior as `ActionController::Parameters#compact_blank!`
collection.reject! { |_k, v| v.blank? }   # Same behavior as `ActionController::Parameters#compact_blank!`

# good
collection.compact_blank!

# bad
tag(:p)
tag(:br, class: 'classname')

# good
tag.p
tag.br(class: 'classname')
tag(name, class: 'classname')

# 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

# good
create_table :users, articles, id: false do |t|
  t.integer :user_id
  t.integer :article_id
end

# bad
add_column :users, :save

# good
add_column :users, :saved

# 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

# bad
def bar
  self.bar
end

# good
delegate :bar, to: :self

# 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.errors[:name] << 'msg'
user.errors.messages[:name] << 'msg'

# good
user.errors.add(:name, 'msg')

# bad
user.errors[:name].clear
user.errors.messages[:name].clear

# good
user.errors.delete(:name)

# bad
user.errors.keys.include?(:attr)

# good
user.errors.attribute_names.include?(:attr)

# bad
I18n.t :record_invalid, scope: [:activerecord, :errors, :messages]
I18n.t :title, scope: :invitation

# good
I18n.t 'activerecord.errors.messages.record_invalid'
I18n.t :record_invalid, scope: 'activerecord.errors.messages'

# bad
belongs_to :foo
belongs_to :bar
has_one :foo

# good
belongs_to :bar
has_one :foo

# bad
has_many :foo, class_name: 'Foo'
has_many :bar, class_name: 'Foo'
has_one :baz

# good
has_many :bar, class_name: 'Foo'
has_one :foo

# bad
scope :visible, -> { where(visible: true) }
scope :hidden, -> { where(visible: true) }

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

# bad
Time.current - 1.minute
Time.current + 2.days

# good - using relative would make it harder to express and read
Date.yesterday + 3.days
created_at - 1.minute
3.days - 1.hour

# good
1.minute.ago
2.days.from_now

# 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
Rails.logger.debug "The time is #{Time.zone.now}."

# good
Rails.logger.debug { "The time is #{Time.zone.now}." }

# 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.development? || Rails.env.test?

# good
Rails.env.local?

# bad
Rails.env == 'production'

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

# good
Rails.env.production?

# good
Rails.application.config.foo
Rails.application.config.x.foo.bar
Rails.application.secrets.foo
Rails.application.config.foo = "bar"

# bad
exit(0)

# good
raise 'a bad error has happened'

# bad
date.beginning_of_day..date.end_of_day
date.beginning_of_week..date.end_of_week
date.beginning_of_month..date.end_of_month
date.beginning_of_quarter..date.end_of_quarter
date.beginning_of_year..date.end_of_year

# good
date.all_day
date.all_week
date.all_month
date.all_quarter
date.all_year

# bad
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
travel_to(Time.now)
travel_to(Time.new)
travel_to(DateTime.now)
travel_to(Time.current)
travel_to(Time.zone.now)
travel_to(Time.now.in_time_zone)
travel_to(Time.current.to_time)

# good
freeze_time

# 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 :articles, dependent: nil
  has_many :patients, through: :appointments
end

class User < ActiveRecord::Base
  has_many :comments
  has_one :avatar

  def readonly?
    true
  end
end

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

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

# good
class MyFormBuilder < ActionView::Helpers::FormBuilder
  @template.do_something
end

# bad
get :new, { user_id: 1}

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

# bad
I18n.locale = :fr

# good
I18n.with_locale(:fr) do
end

# bad
class User < ApplicationRecord
  validates :email, presence: { message: "must be present" }
end

# good
# config/locales/en.yml
# en:
#   activerecord:
#     errors:
#       models:
#         user:
#           blank: "must be present"

class User < ApplicationRecord
  validates :email, presence: true
end

# bad
class PostsController < ApplicationController
  def create
    # ...
    redirect_to root_path, notice: "Post created!"
  end
end

# good
# config/locales/en.yml
# en:
#   posts:
#     create:
#       success: "Post created!"

class PostsController < ApplicationController
  def create
    # ...
    redirect_to root_path, notice: t(".success")
  end
end

# bad
class UserMailer < ApplicationMailer
  def welcome(user)
    mail(to: user.email, subject: "Welcome to My Awesome Site")
  end
end

# good
# config/locales/en.yml
# en:
#   user_mailer:
#     welcome:
#       subject: "Welcome to My Awesome Site"

class UserMailer < ApplicationMailer
  def welcome(user)
    mail(to: user.email)
  end
end

# bad
class User < ActiveRecord::Base
  self.ignored_columns = [:one]
end

# bad
class User < ActiveRecord::Base
  self.ignored_columns = [:one, :two]
end

# good
class User < ActiveRecord::Base
  self.ignored_columns += [:one, :two]
end

# good
class User < ActiveRecord::Base
  self.ignored_columns += [:one]
  self.ignored_columns += [:two]
end

# 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].to_h { |el| [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].to_h { |el| [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 behavior 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

# db/migrate/20220224111111_create_users.rb

# bad
class SellBooks < ActiveRecord::Migration[7.0]
end

# good
class CreateUsers < ActiveRecord::Migration[7.0]
end

# 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
scope :chronological, -> { order(id: :asc) }
scope :chronological, -> { order(primary_key => :asc) }

# good
scope :chronological, -> { order(created_at: :asc) }

# 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)

# Original code
User.select('name AS nickname').map { |user| user[:nickname] } # => array of nicknames

# After autocorrection
User.select('name AS nickname').pluck(:nickname) # => raises ActiveRecord::StatementInvalid

# 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))
Post.where(user_id: User.active.ids)
Post.where.not(user_id: User.active.pluck(:id))

# good
Post.where(user_id: User.active.select(:id))
Post.where(user_id: active_users.select(:id))
Post.where.not(user_id: active_users.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

# good
def foo
  bar || read_attribute(:foo)
end

# bad
User.all.find(id)
User.all.order(:created_at)
users.all.where(id: ids)
user.articles.all.order(:created_at)

# good
User.find(id)
User.order(:created_at)
users.where(id: ids)
user.articles.order(:created_at)

# 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
belongs_to :user
validates :user, presence: true

# bad
belongs_to :user
validates :user_id, presence: true

# bad
belongs_to :author, foreign_key: :user_id
validates :user_id, presence: true

# good
belongs_to :user

# good
belongs_to :author, foreign_key: :user_id

# 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
def teardown
  do_something
  travel_back
end

# good
def teardown
  do_something
end

# bad
after do
  do_something
  travel_back
end

# good
after do
  do_something
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
require_dependency 'some_lib'

# bad
JSON.parse(response.body)

# bad
Nokogiri::HTML.parse(response.body)

# bad
Nokogiri::HTML5.parse(response.body)

# good
response.parsed_body

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

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

# good
def change
  create_table :users do |t|
    t.string :name
  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

# remove_columns

# bad
def change
  remove_columns :users, :name, :email
end

# good
def change
  reversible do |dir|
    dir.up do
      remove_columns :users, :name, :email
    end

    dir.down do
      add_column :users, :name, :string
      add_column :users, :email, :string
    end
  end
end

# good (Rails >= 6.1, see https://github.com/rails/rails/pull/36589)
def change
  remove_columns :users, :name, :email, type: :string
end

# remove_index

# bad
def change
  remove_index :users, name: :index_users_on_email
end

# good
def change
  remove_index :users, :email
end

# good
def change
  remove_index :users, column: :email
end

# bad
class SomeMigration < ActiveRecord::Migration[6.0]
  def up
    # up migration
  end

  # <----- missing down method
end

class SomeMigration < ActiveRecord::Migration[6.0]
  # <----- missing up method

  def down
    # down migration
  end
end

# good
class SomeMigration < ActiveRecord::Migration[6.0]
  def change
    # reversible migration
  end
end

# good
class SomeMigration < ActiveRecord::Migration[6.0]
  def up
    # up migration
  end

  def down
    # down migration
  end
end

# bad
Rails.root.join('db').join('schema.rb')
Rails.root.join('db').join(migrate).join('migration.rb')
Rails.public_path.join('path').join('file.pdf')
Rails.public_path.join('path').join(to).join('file.pdf')

# good
Rails.root.join('db', 'schema.rb')
Rails.root.join('db', migrate, 'migration.rb')
Rails.public_path.join('path', 'file.pdf')
Rails.public_path.join('path', to, 'file.pdf')

# bad
File.open(Rails.root.join('db', 'schema.rb'))
File.open(Rails.root.join('db', 'schema.rb'), 'w')
File.read(Rails.root.join('db', 'schema.rb'))
File.binread(Rails.root.join('db', 'schema.rb'))
File.write(Rails.root.join('db', 'schema.rb'), content)
File.binwrite(Rails.root.join('db', 'schema.rb'), content)

# good
Rails.root.join('db', 'schema.rb').open
Rails.root.join('db', 'schema.rb').open('w')
Rails.root.join('db', 'schema.rb').read
Rails.root.join('db', 'schema.rb').binread
Rails.root.join('db', 'schema.rb').write(content)
Rails.root.join('db', 'schema.rb').binwrite(content)

# bad
Rails.root.join('public')
Rails.root.join('public/file.pdf')
Rails.root.join('public', 'file.pdf')

# good
Rails.public_path
Rails.public_path.join('file.pdf')
Rails.public_path.join('file.pdf')

foo&.blank? #=> nil
foo.blank? #=> true

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

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

# Original code
def update_attributes
end

update_attributes

# After running rubocop --safe-autocorrect
def update_attributes
end

update

# 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 (no comment for a new column or table)
add_column :table, :column, :integer

create_table :table do |t|
  t.type :column
end

# good
add_column :table, :column, :integer, comment: 'Number of offenses'

create_table :table, comment: 'Table of offenses data' do |t|
  t.type :column, comment: 'Number of offenses'
end

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

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

# bad
Model.select(:column_name).map(&:column_name)

# good
Model.pluck(:column_name)

# 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
<<-SQL
  SELECT * FROM posts;
SQL

<<-SQL
  SELECT * FROM posts
    WHERE id = 1
SQL

execute(<<~SQL, "Post Load")
  SELECT * FROM posts
    WHERE post_id = 1
SQL

# good
<<-SQL.squish
  SELECT * FROM posts;
SQL

<<~SQL.squish
  SELECT * FROM table
    WHERE id = 1
SQL

execute(<<~SQL.squish, "Post Load")
  SELECT * FROM posts
    WHERE post_id = 1
SQL

# bad
<<EOS.strip_heredoc
  some text
EOS

# bad
<<-EOS.strip_heredoc
  some text
EOS

# good
<<~EOS
  some text
EOS

# bad
self.table_name = 'some_table_name'
self.table_name = :some_other_name

# bad
add_column :users, :active, :boolean
t.column :active, :boolean
t.boolean :active

# good
add_column :users, :active, :boolean, default: true, null: false
t.column :active, :boolean, default: true, null: false
t.boolean :active, default: true, null: false

# bad
Time.now
Time.parse('2015-03-02T19:05:37')
'2015-03-02T19:05:37'.to_time

# good
Time.current
Time.zone.now
Time.zone.parse('2015-03-02T19:05:37')
Time.zone.parse('2015-03-02T19:05:37Z') # Respect ISO 8601 format with timezone specifier.

# bad
Time.zone = 'EST'

# good
Time.use_zone('EST') do
end

# bad
obj.to_s(:delimited)

# good
obj.to_formatted_s(:delimited)

# bad
HashWithIndifferentAccess.new(foo: 'bar')

# good
ActiveSupport::HashWithIndifferentAccess.new(foo: 'bar')

# bad
ApplicationRecord.transaction do
  return if user.active?
end

# bad
ApplicationRecord.transaction do
  break if user.active?
end

# bad
ApplicationRecord.transaction do
  throw if user.active?
end

# bad, as `with_lock` implicitly opens a transaction too
user.with_lock do
  throw if user.active?
end

# bad, as `with_lock` implicitly opens a transaction too
ApplicationRecord.with_lock do
  break if user.active?
end

# good
ApplicationRecord.transaction do
  # Rollback
  raise "User is active" if user.active?
end

# good
ApplicationRecord.transaction do
  # Commit
  next if user.active?
end