【Rails】初学者のためのコード品質改善ガイド

はじめに

「動くコード」と「良いコード」の間には大きな差があります！
本記事では、Rails初学者がやってしまいがちな問題のある実装と、その改善方法を具体的なコード例とともに紹介していきたいと思います！
もし間違いなどがあればご指摘お願いします。

よくある問題とコード例を提示し、その後に改善ポイントと改善例とを紹介していきます。

1. RESTfulな設計から逸脱したカスタムアクション

❌ よくある問題

まずは独自のアクションを作ってしまうパターンです。
独自のアクションの何が良くないかというと、分かりにくくなってしまうからです。

Railsには7つの基本アクション（index, show, new, create, edit, update, destroy）が存在しています。
これらを利用することで、実装者以外がソースを見た場合でも「どこに何の実装が存在しているか」が予測できます。
これがRailsの良さであり、カスタムアクションはこの良さを捨てることになってしまいます。

# config/routes.rb
resources :tasks do
  collection do
    get :task_list_page
    post :add_task
    get :task_edit_page
    patch :modify_task
    get :task_detail_page
    delete :remove_task
  end
end

📝 改善ポイント

• Railsの7つの基本アクション（index, show, new, create, edit, update, destroy）で表現できないか、まず考える
• カスタムアクションが必要な場合は、本当に必要か再検討する
• 新しいリソースとして切り出せないか検討する
  （例：tasks/completions_controller.rb）

✅ 改善例

# config/routes.rb
resources :tasks
# これだけで index, show, new, create, edit, update, destroy が使える

---

2. コントローラーの肥大化（Fat Controller）

❌ よくある問題

コントローラーにビジネスロジックを詰め込みすぎています。
ビジネスロジックとは、アプリケーション固有の業務ルールや処理のことです。
下記の例でいえば、「日付で絞り込む」「ステータスで絞り込む」「月別に集計する」といった処理がこれにあたります。
これらは「注文データをどう扱うか」というビジネス上の関心事であり、「HTTPリクエストを受けてレスポンスを返す」というコントローラーの責務とは本来別のものです。
コントローラーにビジネスロジックを書いてしまうと、以下の問題が起きます。

• コードが分かりづらい
  アクションが長くなり、何をしているか把握しづらくなってしまいます。
• テストが書きにくい
  1つのブロックが大きくなってしまうので、テストが難しくなってしまいます。

# app/controllers/orders_controller.rb
class OrdersController < ApplicationController
  def index
    @orders = current_user.orders
    
    # 日付フィルタ
    if params[:start_date].present? && params[:end_date].present?
      start_date = Date.parse(params[:start_date])
      end_date = Date.parse(params[:end_date])
      @orders = @orders.where(created_at: start_date.beginning_of_day..end_date.end_of_day)
    end
    
    # ステータスフィルタ
    if params[:status] == "completed"
      @orders = @orders.where(completed: true).where.not(completed_at: nil)
    elsif params[:status] == "pending"
      @orders = @orders.where(completed: false)
    elsif params[:status] == "cancelled"
      @orders = @orders.where(cancelled: true)
    end
    
    # 合計金額の計算
    @total_amount = @orders.sum(:amount)
    
    # 月別集計
    @monthly_totals = @orders.group("DATE_TRUNC('month', created_at)")
                         .sum(:amount)
    
    # ページネーション
    @orders = @orders.order(created_at: :desc).page(params[:page]).per(20)
  end
end

📝 改善ポイント

• モデルにスコープを定義して、クエリロジックをコントローラーから分離する
• コントローラーは「何をするか」を指示する役割に徹する

✅ 改善例

# app/models/order.rb
class Order < ApplicationRecord
  belongs_to :user

  # スコープで絞り込みロジックを定義
  scope :in_date_range, ->(start_date, end_date) {
    where(created_at: start_date.beginning_of_day..end_date.end_of_day)
  }
  scope :completed, -> { where(completed: true).where.not(completed_at: nil) }
  scope :pending, -> { where(completed: false) }
  scope :cancelled, -> { where(cancelled: true) }

  scope :by_status, ->(status) {
    case status
    when "completed" then completed
    when "pending"   then pending
    when "cancelled" then cancelled
    else all
    end
  }

  def self.monthly_stats
    group("DATE_TRUNC('month', created_at)").sum(:amount)
  end
end

# app/controllers/orders_controller.rb
class OrdersController < ApplicationController
  def index
    @orders = current_user.orders.by_status(params[:status])
    @orders = apply_date_filter(@orders)
    @total_amount = @orders.sum(:amount)
    @monthly_stats = @orders.monthly_stats
    @orders = @orders.order(created_at: :desc).page(params[:page]).per(20)
  end

  private

  def apply_date_filter(scope)
    return scope unless params[:start_date].present? && params[:end_date].present?
    
    scope.in_date_range(Date.parse(params[:start_date]), Date.parse(params[:end_date]))
  end
end

---

3. マジックナンバー・ハードコーディング

❌ よくある問題

24や4などの数値、confirmedなどの文字列が直接コードに書かれています。
こちらの問題は以下の通りです。

• 意味が分かりづらい
  コードを読んだ人が「24って何？」「4って何の制限？」となってしまいます。
• 変更時に漏れが発生する
  同じ数値が複数箇所にあると、変更時に一部だけ直し忘れるリスクがあります。
• 文字列の比較はtypoに弱い
  "confirmed" を "comfirmed" と書いてもエラーにならず、バグの原因になります。

# app/controllers/reservations_controller.rb
def create
  # 24時間以上前の予約のみ受付
  if params[:reserved_at].to_time < Time.current + 24.hours
    redirect_to new_reservation_path, alert: "24時間以上先の日時を指定してください"
    return
  end

  # 1回の予約は最大4名まで
  if params[:guests].to_i > 4
    redirect_to new_reservation_path, alert: "最大4名までです"
    return
  end

  # ステータス判定
  if @reservation.status == "confirmed"
    # 処理...
  end
end

📝 改善ポイント

• 数値は定数として意味のある名前をつける
• ステータスなどの状態管理にはenumを使う
• バリデーションはモデルに集約する（コントローラーで個別チェックしない）

✅ 改善例

# app/models/reservation.rb
class Reservation < ApplicationRecord
  # 定数として定義
  MINIMUM_ADVANCE_HOURS = 24
  MAXIMUM_GUESTS = 4

  # enumで状態を管理
  enum :status, {
    pending: 0,
    confirmed: 1,
    cancelled: 2
  }

  # バリデーションとして定義
  validate :must_be_future_reservation
  validates :guests, numericality: { 
    less_than_or_equal_to: MAXIMUM_GUESTS,
    message: "は最大#{MAXIMUM_GUESTS}名までです"
  }

  private

  def must_be_future_reservation
    return if reserved_at.blank?
    
    minimum_time = Time.current + MINIMUM_ADVANCE_HOURS.hours
    if reserved_at < minimum_time
      errors.add(:reserved_at, "は#{MINIMUM_ADVANCE_HOURS}時間以上先の日時を指定してください")
    end
  end
end

# app/controllers/reservations_controller.rb
def create
  @reservation = current_user.reservations.build(reservation_params)

  if @reservation.save
    redirect_to @reservation, notice: "予約が完了しました"
  else
    render :new, status: :unprocessable_entity
  end
end

---

4. ネストが深い条件分岐

❌ よくある問題

ネストが深くなってしまっています。

• 可読性が低い
  読む人の頭の中で「今どの条件の中にいるのか」を常に考えておく必要があり、認知負荷が高くなります。
• テストケースが複雑になる
  条件の組み合わせが増え、網羅的なテストが難しくなります。

# app/controllers/checkouts_controller.rb
def checkout
  if current_user.present?
    if @cart.items.any?
      if @cart.total_price >= 1000
        if current_user.payment_method.present?
          # 決済処理
          @order = Order.create!(user: current_user, total: @cart.total_price)
          redirect_to @order, notice: "注文が完了しました"
        else
          redirect_to payment_methods_path, alert: "支払い方法を登録してください"
        end
      else
        redirect_to cart_path, alert: "1000円以上からご注文いただけます"
      end
    else
      redirect_to cart_path, alert: "カートが空です"
    end
  else
    redirect_to login_path, alert: "ログインしてください"
  end
end

📝 改善ポイント

• ガード節（早期リターン）を使って異常系を先に処理する
• 正常系の処理がネストなしで読める
• 各条件が独立しているので、追加・変更が容易

✅ 改善例（ガード節を使用）

# app/controllers/checkouts_controller.rb
def checkout
  if current_user.nil?
    redirect_to login_path, alert: "ログインしてください"
    return
  end

  if @cart.items.empty?
    redirect_to cart_path, alert: "カートが空です"
    return
  end

  if @cart.total_price < Order::MINIMUM_ORDER_AMOUNT
    redirect_to cart_path, alert: "#{Order::MINIMUM_ORDER_AMOUNT}円以上からご注文いただけます"
    return
  end

  if current_user.payment_method.blank?
    redirect_to payment_methods_path, alert: "支払い方法を登録してください"
    return
  end

  # 正常系の処理（ネストなし）
  @order = Order.create!(user: current_user, total: @cart.total_price)
  redirect_to @order, notice: "注文が完了しました"
end

5. 不要なインスタンス変数の使用

❌ よくある問題

@user = current_user のように、すでに利用可能なヘルパーメソッドをわざわざインスタンス変数に代入しています。
この書き方には以下の問題があります。

• 冗長なコードになる
  既に使えるものを別名で定義し直す意味がありません。
• 変数の追跡が必要になる
  「この @user はどこで設定されている？」とコードを遡る手間が発生します。

# app/controllers/profiles_controller.rb
def show
  @user = current_user
  @posts = @user.posts
end

<!-- app/views/profiles/show.html.erb -->
<h1><%= @user.name %>さんのプロフィール</h1>

📝 改善ポイント

• current_user はヘルパーメソッドなので、Viewから直接参照できる
• 不要な変数代入は可読性を下げる
• 本当にViewで必要なデータだけをインスタンス変数にする

✅ 改善例

# app/controllers/profiles_controller.rb
def show
  @posts = current_user.posts
end

<!-- app/views/profiles/show.html.erb -->
<h1><%= current_user.name %>さんのプロフィール</h1>

---

6. 同じ処理の重複

❌ よくある問題

「未ログインユーザーには非公開記事を見せない」という同じロジックが複数のアクションに書かれています。
この書き方には以下の問題があります。

• 修正漏れが発生しやすい
  仕様変更時に一箇所だけ直して、他の箇所を忘れるリスクがあります。
• コード量が増える
  同じ処理を何度も書くことで、コントローラーが肥大化します。
• ロジックの不整合が起きやすい
  コピペで書いていると、微妙に条件が違うバグが生まれることがあります。

# app/controllers/articles_controller.rb
class ArticlesController < ApplicationController
  def index
    # 未ログインユーザーに非公開記事は見せない
    @articles = if current_user
                  Article.where(user_id: current_user.id)
                        .or(Article.where(published: true))
                        .order(created_at: :desc)
                else
                  Article.where(published: true).order(created_at: :desc)
                end
  end

  def search
    # 未ログインユーザーに非公開記事は見せない（同じロジックの重複）
    base = if current_user
             Article.where(user_id: current_user.id)
                   .or(Article.where(published: true))
           else
             Article.where(published: true)
           end
    
    @articles = base.where("title LIKE ?", "%#{params[:q]}%").order(created_at: :desc)
  end
end

📝 改善ポイント

• DRY原則（Don't Repeat Yourself） を意識する
• 共通ロジックはスコープやメソッドとして抽出する
• 同じ処理が2箇所以上に出現したら、共通化を検討する

✅ 改善例

# app/models/article.rb
class Article < ApplicationRecord
  scope :published, -> { where(published: true) }
  scope :visible_to, ->(user) {
    return published unless user
    where(user_id: user.id).or(published)
  }
end

# app/controllers/articles_controller.rb
class ArticlesController < ApplicationController
  def index
    @articles = Article.visible_to(current_user).order(created_at: :desc)
  end

  def search
    @articles = Article.visible_to(current_user)
                       .where("title LIKE ?", "%#{params[:q]}%")
                       .order(created_at: :desc)
  end
end

---

7. トランザクションの欠如

❌ よくある問題

複数のレコードを更新する処理で、トランザクションが使われていません。
この書き方には以下の問題があります。

• データの不整合が発生する
  例えば、メンバー追加は成功したのに通知作成で失敗すると、中途半端な状態でデータが残ります。

# app/controllers/invitations_controller.rb
def accept_invitation
  # 招待を承認
  @invitation.update!(accepted_at: Time.current)
  
  # チームにメンバーを追加
  @invitation.team.members.create!(user: current_user)
  
  # 通知を作成
  Notification.create!(
    user: @invitation.inviter,
    message: "#{current_user.name}さんが招待を承認しました"
  )

  redirect_to team_path(@invitation.team), notice: "チームに参加しました"
end

📝 改善ポイント

• 複数のレコードを更新する場合は transaction で囲む
• どれか一つでも失敗したら、すべてロールバックされる
• データの整合性を保つために必須

✅ 改善例

# app/controllers/invitations_controller.rb
def accept_invitation
  ActiveRecord::Base.transaction do
    @invitation.update!(accepted_at: Time.current)
    @invitation.team.members.create!(user: current_user)
    Notification.create!(
      user: @invitation.inviter,
      message: "#{current_user.name}さんが招待を承認しました"
    )
  end

  redirect_to team_path(@invitation.team), notice: "チームに参加しました"
rescue ActiveRecord::RecordInvalid => e
  redirect_to invitations_path, alert: "参加に失敗しました: #{e.message}"
end

---

8. 意味のないコメント / コメントの欠如

❌ よくある問題：コードをそのまま説明するコメント

コードを読めば分かることをそのままコメントに書いています。
この書き方には以下の問題があります。

• プラスの情報がない
  簡単なコードと同じことを日本語で繰り返しているだけで、プラスの情報がありません。
• メンテナンスコストが増える
  コードを変更したらコメントも修正が必要になり、二重管理になります。

# app/models/order.rb
def calculate_total
  # itemsの合計を計算
  total = items.sum(:price)
  
  # 税率をかける
  total_with_tax = total * 1.1
  
  # 結果を返す
  total_with_tax
end

📝 ポイント

• コードを読めばわかることはコメントにしない
• なぜそうしているかをコメントに書く
• 定数や変数名で意図を表現できればコメント不要

✅ 改善例

# app/models/order.rb
TAX_RATE = 1.1
MEMBER_DISCOUNT_RATE = 0.9

def calculate_total
  subtotal = items.sum(:price)
  total_with_tax = subtotal * TAX_RATE
  
  # 会員は10%割引を適用
  member? ? total_with_tax * MEMBER_DISCOUNT_RATE : total_with_tax
end

---

9. 命名の一貫性がない

❌ よくある問題

同じ概念を表すのに、場所によって異なる名前が使われています。
この書き方には以下の問題があります。

• 予測しづらい
  「日付の範囲指定は start_date？ from？ begin_date？」と毎回確認が必要になります。
• 検索しづらい
  start_date と from と date_begin が混在していると、grep や検索で漏れが発生します。

# 同じ「期間指定」なのに命名がバラバラ
class ReportsController < ApplicationController
  def sales
    start_date = params[:start_date]
    end_date = params[:end_date]
    # ...
  end
end

class AnalyticsController < ApplicationController
  def index
    from = params[:from]
    to = params[:to]
    # ...
  end
end

class OrdersController < ApplicationController
  def search
    date_begin = params[:date_begin]
    date_end = params[:date_end]
    # ...
  end
end

📝 改善ポイント

• 命名規則を統一する
• 同じ概念には同じ名前を使う（from / start_date / date_begin の混在を避ける）

✅ 改善例

# プロジェクト全体で命名を統一
class ReportsController < ApplicationController
  def sales
    start_date = params[:start_date]
    end_date = params[:end_date]
    # ...
  end
end

class AnalyticsController < ApplicationController
  def index
    start_date = params[:start_date]
    end_date = params[:end_date]
    # ...
  end
end

class OrdersController < ApplicationController
  def search
    start_date = params[:start_date]
    end_date = params[:end_date]
    # ...
  end
end

---

10. モデルのスコープ・メソッドを使わない

❌ よくある問題

モデルにスコープやメソッドが定義されているのに、コントローラーで同じクエリを直接書いています。
中途半端にAIを利用していると発生しやすいような気がします。

• コードの重複
  同じクエリ条件を複数箇所に書くことになり、DRY原則に反します。
• 仕様変更に弱い
  「公開」の条件が変わったとき、スコープとコントローラーの両方を修正する必要があります。

# app/models/post.rb
class Post < ApplicationRecord
  scope :published, -> { where(published: true) }
  scope :recent, -> { order(created_at: :desc) }
  
  def self.featured
    where(featured: true).published.recent.limit(5)
  end
end

# app/controllers/posts_controller.rb
class PostsController < ApplicationController
  def index
    # せっかくスコープがあるのに使っていない
    @posts = Post.where(published: true).order(created_at: :desc)
  end

  def featured
    # モデルのメソッドを使わずに同じロジックを書いている
    @posts = Post.where(featured: true)
                 .where(published: true)
                 .order(created_at: :desc)
                 .limit(5)
  end
end

📝 改善ポイント

• 定義したスコープやメソッドは積極的に使う
• コードの意図が明確になり、可読性が向上する
• 変更時に1箇所の修正で済む

✅ 改善例

# app/controllers/posts_controller.rb
class PostsController < ApplicationController
  def index
    @posts = Post.published.recent
  end

  def featured
    @posts = Post.featured
  end
end

---

おわりに

いかがだったでしょうか。
全ては分かりやすく、保守しやすくというところに繋がるので、絶対的な正解はないと思います。
ぜひ自分のコードを振り返ってみてください。
最初から全部を完璧にというようなものではなく、徐々に慣れていきたいですね！
読んで頂きありがとうございました。