可読性のある書き方

##モデル
人の年齢を表すカラムに「歳」を加えたり、重さを表すカラムに「キログラム」を加えるメソッドは、ビューでのみ使用される見た目を整えるロジックです。このような見た目に関わるロジックは、ヘルパーかデコレーターに記述します。

# ☓：ビューでしか使用しないメソッドをモデルに定義している

# 例) Userモデルにcreated_atを年, 月, 日の形に変換するメソッドを定義している
class User < ApplicationRecord
  def adjust_created_at
    created_at.to_date.strftime("%Y年%m月%d日")
  end
end

 # ◯：ヘルパーファイルにビューで使用するロジックを定義している
module ApplicationHelper
  def set_created_at(created_at)
    created_at.to_date.strftime("%Y年%m月%d日")
  end
end

# ◎：ビューで使用するロジックのうち、表示フォーマットを整えるものを、DraperもしくはActiveDecoratorを導入して実装している

# 例) ActiveDecoratorを使用してcreated_atを年, 月, 日の形に変換するメソッドを定義している
module UserDecorator
  def set_created_at
    created_at.to_date.strftime("%Y年%m月%d日")
  end
end

１つのメソッドで多くのことをやりすぎていないかの確認
例として、Userモデルに定義されたクラスメソッドchange_attributesが、tweet, commentの変更まで行なってしまっています。
１つのメソッドで行う処理はなるべく簡潔にまとめます。

# X:Userモデルに定義された、change_attributesメソッドが、Tweet, Commentまで変更してしまっている
class User < ApplicationRecord
  def self.change_attributes(user, tweet, comment)
    user.attributes = { family_name: "Hoge", first_name: "Fuga" }
    tweet.attributes = { body: Foo }
    comment.attributes = { body: Bar}
  end
end

ロジックをスコープとしてモデルに定義します。

#  ◯:コントローラでよく使用するロジックをスコープとしてモデルに定義している

class User < ApplicationRecord
# 〜省略〜
 scope :team_red, -> { where(team: 0) }
 scope :captain, ->{ where(captain: true) }
end

# 上記のスコープにより、User.team_red.captainのように直感的にロジックを記述できる

##コントローラ
不必要な自作アクションの確認
基本的にはrailsの7つのアクションで実装します。
アクションの内部の記述が複雑になってしまう場合は、モデルやモジュールを活用して、別のファイルに処理を移します。

# X:makeアクションはcreateとやっていることが変わらない

def create
  @user.create(create_params)
end

def make
  @user.create(create_params)
end

# ◯:なるべく7つのアクションの範囲でルーティングを定義する

def create
  @user.create(create_params)
end

N+1問題は起こっていないかを確認します。
has_many, belongs_toなどアソシエーションを設定しているモデルを扱う際には、includeなどの対策を忘れずに行います。

# X:1対Nの関係で、includeをしていない
# 1つのCompanyが複数のEmployeeをもっている例
# includeをしないとEmployeeの数だけCompanyをSELECTするSQLが発行されるため、レコードが増えるほど重くなる

def index
  @employees = Employee.all 
end

# ◯:変数を定義する際にincludeしている
# includes(:employee)と記述することで、SQLの発行回数を抑えて動作を軽くできる

def index
  @employees = Employee.all .includes(:employee)
end

##ビュー
部分テンプレートの活用
特定のビューを繰り返したい時は、eachより動作の軽い部分テンプレートを活用します。

# X:レコードの数だけ同じビューを繰り返す部分で、eachを使用している
h2
- @users.each do |user|
  .user-container
    = user.name
    = user.profile
    = user.image
end

# ◯:繰り返しを部分テンプレートで記述している
# 部分テンプレートを使用する理由は動作が軽いから
h2
  = render partial: 'user-contaier', collection @users

##ルーティング
不要なルーティングを定義していないかの確認
不要なルーティングを定義してしまうと、万が一該当するパスにユーザーが遷移した時に500エラーが表示されてしまいます。また、意図せずupdate, destroyなどのメソッドが動いてしまう危険性もあるため、必要なルーティングのみを定義するようにします。

# X : indexしか必要ないのに、resourcesで丸ごと定義している
resources :items

# ◯:only, exceptを使用して必要なルーティングのみ定義している
resources: items, only: :index

##可読性
スペースの数、ネストの深さの確認

# X:スペースの数がバラバラになっている
# fugafugaもhogehogeも同じクラスのインスタンスメソッドなのに、スペースの数が多い

def hogehoge
  puts "こんにちは"
end

  def fugafuga
    puts "Hello"
  end

# ◯:スペースの数が揃っている

def hogehoge
  puts "こんにちは"
end

def fugafuga
  puts "Hello"
end

##命名規則
変数の名前は適切か
変数の名前は、一目見れば変数の中身が分かるような命にします。

// X:アルファベット1文字
// ※Javascriptのローカル変数ではたまに1文字だけの変数名を定義するが、rubyではあまり使わない

p = Person.first
s = Ship.all

// X:変数の中身が分からない名前
// 「一体どのモデルのeverythingなのか？」となってしまい、変数の目的が伝わらない

@everything = Tweet.all

// X:略語
// 略語は元の単語を想像しづらく、略し方も人それぞれ異なってしまうので使うべきでない
// 変数の中身が単数なのか複数なのかも分かりづらい

@au = User.admin

// ◯：
first_person = Person.first
ships = Ship.all

@tweets = Tweet.all

@admin_users  = User.admin

キャメルケースとスネークケースを使い分けているか

# キャメルケースは先頭が小文字で、単語の区切りを大文字で表す
# アッパーキャメルケースは先頭から大文字を使用する

キャメルケース: adminUserCommentCreator
アッパーキャメルケース: AdminUserCommentCreator


# スネークケースは単語の区切りをアンダースコアで表す
スネークケース: admin_user_comment_creator


Railsの慣習的な命名規則として、下記のように使い分ける。

クラス名：アッパーキャメルケース
メソッド名：スネークケース
変数名：スネークケース

また、Javascriptでは一般的にキャメルケースを用いて記述する。

ハイフンとアンダースコアを使い分けとBEMでクラス名を定義できているかの確認
ハイフンとアンダースコアを明確に使い分けて命名を行うと、可読性が大きく向上します。CSSも書きやすくなる、BEM記法を用いてクラス名を定義します。

// X:ハイフンとアンダーバーを使い分けていない

.review_box
  %ul.reviews-list
   %li.review_1
   %li.review_2
   %li.review_3_red
   %li.review_4


// ◯:ハイフンとアンダーバーを使い分け、BEMでクラス名を定義している
// 親要素のクラス名からアンダースコアを２つ並べて、クラス名を定義している
// 同じ要素のうち、差分を定義するクラス名をハイフンを使って定義している(-red)

.reviews
  %ul.reviews__list
    %li.reviews__list__row
    %li.reviews__list__row    
    %li.reviews__list__row.reviews__list__row-red
    %li.reviews__list__row

##設計
単一責任原則
単一責任原則とは、あるクラスを変更するべき理由は2つ以上あるべきでないというオブジェクト志向の原則の１つです。当該原則を満たしていないコードは、バグの原因となりやすく、将来の変更にも弱くなってしまいます。
参考サイト

# ☓:Gearクラスが車輪に関係するリム、タイヤの情報を持っている

# 自転車のギアを表すGearクラスが、「車輪のリムの直径、タイヤの厚み」まで責任を持ってしまっている
# Wheelクラスを作り、rim, tireの定義を移すべき

class Gear
  attr_reader :chainring, :cog, :rim, :tire
  def initialize(chainring, cog, rim, tire)
    @chainring = chainring
    @cog       = cog
    @rim       = rim
    @tire      = tire
  end

  def ratio
    chainring / cog.to_f
  end

  def gear_inches
    ratio * (rim + (tire * 2))
  end
end

# 下記のコードはギアに関する情報しか渡していないにも関わらずエラーになる
puts Gear.new(52, 11).ratio
# ArgumentError: wrong number of arguments (2 for 4)


# ◯:Wheelクラスを作り、車輪に関する定義を移す

class Gear
  attr_reader :chainring, :cog, :wheel
  def initialize(chainring, cog, wheel=nil)
    @chainring = chainring
    @cog       = cog
    @wheel     = wheel
  end

  def ratio
    chainring / cog.to_f
  end

  def gear_inches
    ratio * wheel.diameter
  end
end

class Wheel
  attr_reader :rim, :tire

  def initialize(rim, tire)
    @rim       = rim
    @tire      = tire
  end

  def diameter
    rim + (tire * 2)
  end

end


@wheel = Wheel.new(26, 1.5)
puts @wheel.circumference
# -> 91.106186954104

puts Gear.new(52, 11, @wheel).gear_inches
# -> 137.090909090909

単一責任原則に違反していないかチェックする方法
(1)クラスの持つメソッドを質問に置き換える

「Gearさん、あなたのgear_inchesを教えてください」
# Gearにギアに関する質問をしているのでセーフ

「Gearさん、タイヤの厚みとリムの直径を教えてください」
# Gearと無関係なタイヤの質問をしているのでアウト
# 該当する定義、メソッドを別クラスに切り出すべき

(2)1つの文章でクラスを説明してみる

「Gearクラスはチェーンリングとコグ、その比率を持っています」
# ギアの役割を短い文章で表現できているのでセーフ

「Gearクラスはチェーンリングとコグ、その比率を持っており、更に関連した車輪のタイヤの厚みとリムの直径を持っていて、ギアインチを計算することがで計算することができます」

# 文章が長くなりすぎている
# 上手く文章にできない場合は、1つのクラスで多くのことをやりすぎている可能性が高い

マジックナンバーを使用していないかの確認
変数を定義する際、メソッドで数値をやり取りする際などに、生のまま扱われている数字をマジックナンバーといいます。
マジックナンバーを含むコードは、数字を見ただけでは「その数字が何を意味するのか」が分からないため、コメントアウトで数字の説明をする、定数を使う、といった工夫をして、なるべく数字に意味を持たせます。

# X:インスタンスを作成する際にマジックナンバーを用いて値を代入している
# コードを見ただけは「type: 1」がどういった商品タイプを示しているのか分からない

Product.create {name: 'hogehoge', type: 1}

# ◯:コメントアウトで数字の説明をしている

# type: 1は日本産の商品
Product.create {name: 'hogehoge', type: 1}


# ◯:定数を定義して値を代入している
# コードを読むだけで「type: 1」が日本に関連した商品であることがわかる

JAPANESE_PRODUCT_TYPE = 1.freeze
Product.create {name: 'hogehoge', type: JAPANESE_PRODUCT_TYPE}

変数にnilや0が入っても大丈夫な構造か
バリデーションをかけていない、null制約を定義していないなどの理由で、変数にnil、0が入ってしまうことがあります。このままnilの変数に対してメソッドの呼び出しを行うと、Undefined method <メソッド名> for for nil:NilClassというエラーが出てしまいます。nilの場合は呼び出しが行われないようにするなどの対策を取ります。

# X:非ログイン時に表示されるビューで、current_user.idを指定している
# current_userはログイン時のみ使用できるdeviseのヘルパーなので、非ログイン時に使おうとするとエラーになる

= link_to "プロフィール", "users/profile/#{current_user.id}"


# ◯:該当箇所がログイン時のみ読み込まれるように条件を設ける

- if user_signed_in?
  = link_to "プロフィール", "users/profile/#{current_user.id}"