「プロを目指すRailsエンジニアのための公開コードレビュー」という発表をしました #railsdm

はじめに

2017年8月24日にRails Developers Meetup #4という勉強会で「プロを目指すRailsエンジニアのための公開コードレビュー」という発表をしてきました。

このエントリではこの勉強会の発表内容を紹介します。

発表のテーマを決めるまでの経緯

以前、「WEB+DB PRESS Vol.99の「良いコード」を本気でコードレビューしてみた」というエントリを書いて結構な反響があったのですが、この記事を読まれた主催者の平野さん（@yoshi_hirano）から「良いコードとは何か、というテーマで体系的に語ってもらいたい」と声をかけてもらったのが登壇のきっかけでした。

ただ、「良いコードを体系的に語る」となると、僕の中ではおそらく「CODE COMPLETEやリーダブルコードを読め。以上！」になってしまうので、それよりも具体的なコード例を見ながら「ここはこう」「このコードはこう」と説明する発表の方がいいだろうなと思いました。

また、具体的なコードを見ながら良し悪しを語るにしても「お互いに知っているコード」でないと、なかなかぱっと理解することができません。
そこで、

僕の方で簡単なプログラミング問題を作る
それをみんなに解いてもらう
さらに、いただいた解答をいくつかピックアップして発表の中でコードレビューする

というスタイルにするのはどうか、と提案しました。

平野さんも「いいですね。面白そう！」と言ってくれたので、ちょっと珍しい「公開コードレビュー」という形式の発表をやることになりました。

CODE COMPLETE 第2版上完全なプログラミングを目指して

作者:スティーブマコネル
日経BP

Amazon

リーダブルコード ―より良いコードを書くためのシンプルで実践的なテクニック (Theory in practice)

作者:Dustin Boswell,Trevor Foucher
オライリージャパン

Amazon

問題の概要

僕が作った問題はTrainTicketRailsという、電車の改札口を簡易的にシミュレートしたRailsアプリです。
あらかじめある程度コードができていて、そこに指定された仕様を満たすコードを書いてもらう、というのが今回のお題になります。

詳しくは以前書いたこちらのエントリをご覧ください。

blog.jnito.com

ちなみにこの問題は2017年11月に発売予定の書籍「プロを目指す人のためのRuby入門」のために作った例題をRails向けにモディファイしたものです。

blog.jnito.com

当日は自宅からリモート登壇！

この勉強会はなんと、僕は自宅から発表する「リモート登壇者」でした。
さらに、登壇者だけでなく、勉強会の会場もメイン会場の東京会場に加えて、東京会場と中継でつなく大阪会場と、自宅から視聴可能なリモート会場もありました。

発表する側も、発表を聞く側も、リモートでつながる勉強会とは何とも未来的ですね～。

ちなみに当日はGoogleハングアウトとYouTubeライブを使ってリモート登壇＆リモート視聴をしていました。
音声や映像が途切れがちになったりしないかちょっと心配でしたが、結構きれいに中継できていたみたいです。

当日の発表スライドはこちら

当日使ったスライドはこちらです。

ただ、当日はGitHub上のpull requestを見ながらお話をしたので、スライドにはあまりコードが出てきません。
そこでこのブログの中でざっくりとレビュー内容を紹介していきます。

一人目の方のコード

最初はこちらの方のコードをレビューさせてもらいました。

https://github.com/JunichiIto/train-ticket-rails/pull/27

コントローラのコードについて

コントローラのコード（tickets_controller.rb）はこんなふうに実装されていました。

   def edit
+    load_ticket
+    redirect_to root_path, notice: '降車済みの切符です。' if @ticket.used?
   end
 
   def update
-    if @ticket.update(ticket_update_params)
-      redirect_to root_path, notice: '降車しました。😄'
+    load_ticket
+    exited_gate = Gate.find(ticket_update_params[:exited_gate_id])
+
+    if @ticket.used?
+      redirect_to root_path, notice: '降車済みの切符です。' if @ticket.used?
+    elsif exited_gate.exit?(@ticket)
+      update_ticket
     else
+      @ticket.errors[:base] << '降車駅 では降車できません。'
       render :edit
     end
   end

 （略）

   def load_ticket
     @ticket = Ticket.find(params[:id])
   end
+
+  def update_ticket
+    if @ticket.update(ticket_update_params)
+      redirect_to root_path, notice: '降車しました。😄'
+    else
+      render :edit
+    end
+  end

こちらのコードに対しては以下のようなコメントをさせてもらいました。

load_ticketメソッドはbefore_actionで呼ばれているので、editやupdateの中で呼び出す必要はない。
"降車済みの切符です。"のメッセージを表示するコードが2回出てきているので、DRYにするためにメソッドに切り出してbefore_actionで呼び出すようにしたい。
Gate.findのようにfindメソッドを使っている部分があるが、ここは@ticket.exited_gateのようにRailsの関連をうまく使ってデータを取ってくるようにしたい。
updateメソッド内にたくさん条件分岐が出てきて、「ファットコントローラ」になっている。ロジックはできるだけモデルに持っていって、「薄いコントローラ」を目指したい。
コントローラでデータのチェックをすると、モデル単体でデータを保存したときに「同じ駅で降りるな」「料金不足なら降車不可」といったデータのチェックを実行できない。確実にデータがチェックされるよう、検証ロジックはモデルに持ってくるべき。

モデルのコードについて

モデルのコード（gate.rb）はこんなふうに実装されていました。

   def exit?(ticket)
-    true
+    gate_interval = fetch_gate_interval(ticket.entered_gate_id)
+
+    # 運賃不足ではないかチェック
+    if gate_interval.zero?
+      false
+    elsif FARES[gate_interval - 1] <= ticket.fare
+      true
+    else
+      false
+    end
+  end
+
+  private
+
+  def fetch_gate_interval(entered_gate_id)
+    (
+      entered_station_number(entered_gate_id) - station_number
+    ).abs
+  end
+
+  def entered_station_number(gate_id)
+    Gate.find(gate_id).station_number
   end

僕の指摘内容は以下のとおりです。

このコードでもGate.findが登場しているが、ticket.entered_gate.station_numberのようにしてRailsの関連を使った方がスマート。
exit?メソッドではtrueとfalseを明示的に返しているが、<=の比較結果をそのまま返せばtrueをfalse明示的に返す必要がない。

二人目の方のコード

二本目はこちらのコードをレビューさせてもらいました。

https://github.com/JunichiIto/train-ticket-rails/pull/15/

コントローラのコードについて

コントローラは次のようになっていました。

   def load_ticket
     @ticket = Ticket.find(params[:id])
+    redirect_to root_path, alert: '降車済みの切符です。' if @ticket.exited_gate_id
   end

僕のコメントは以下のとおりです。

コントローラにほとんど手を加えず、たった1行の変更で済ませた点は非常にスマート（「薄いコントローラ」を維持できている）。
ただし、load_ticketメソッドの中で、データをチェックして問題があればリダイレクトさせるのは、load_ticketメソッドの責務を超えているように思う。
メソッドの責務が増えると再利用性も低下してしまうので、ここは別メソッドに分けてbefore_actionで呼び出す方がベターではないか。

モデルのコードについて

改札口モデル（gate.rb）のコードは次のようになっていました。

   def exit?(ticket)
-    true
+    price = calculate(ticket)
+    price > 0 && ticket.fare - price >= 0
+  end
+
+  private
+
+  def calculate(ticket)
+    section = (station_number - ticket.entered_gate.station_number).abs
+    section > 0 ? FARES[section - 1] : 0
   end

以下は僕からのコメントです。

こちらもシンプルでスマート。ticket.entered_gate.station_numberのように、Railsの関連もちゃんと使えている。
ただし、calculateメソッドが0（ゼロ）を返すところが気になる。この0は「無料だから降車OK」の意味ではなく、「同じ駅で降りようとしているので降車不可」のフラグになってしまっている。
作った人だけがわかる暗黙の意味、暗黙のルールはできるだけ避けるべき。プログラムが大きくなったり、開発する人が増えたりすると、「0の運賃」がそのまま「無料」と解釈される恐れがある。

もう一つ、切符モデル（ticket.rb）にも次のような変更が入っていました。

 class Ticket < ApplicationRecord
   belongs_to :entered_gate, class_name: 'Gate', foreign_key: 'entered_gate_id'
-  belongs_to :exited_gate, class_name: 'Gate', foreign_key: 'exited_gate_id', required: false
+  belongs_to :exited_gate, class_name: 'Gate', foreign_key: 'exited_gate_id', optional: true
   validates :fare, presence: true, inclusion: Gate::FARES
   validates :entered_gate_id, presence: true
+
+  validate :must_be_exit, if: :exited_gate_id
+
+  private
+
+  def must_be_exit
+    errors.add(:exited_gate, 'では降車できません。') unless exited_gate.exit?(self)
+  end
 end

僕のコメントは以下のとおりです。

requiredオプションが非推奨になって、optinalオプションを使うようになっていたのは知らなかった（僕の方が勉強になった！）
must_be_exitメソッドの実装は問題なし。ただし、must_be_exitだと「be動詞 + 一般動詞」になってしまうので、must_be_exitableやcan_exitのようにした方が英文法的には良さそう。

僕の解答例

僕の解答例はGitHubのanswerブランチに置いてあります。

https://github.com/JunichiIto/train-ticket-rails/compare/master...answer

コントローラのコードについて

コントローラのコードは以下のとおりです。

「使用済みのチケットが指定されたらトップページへリダイレクト」のコードはbefore_actionを使って実現しています。

 class TicketsController < ApplicationController
   before_action :load_ticket, only: %i(edit update show)
+  before_action :abort_exited_ticket, only: %i(edit update)
 
   def index
     redirect_to root_path

 （略）
 
   private
 
+  def abort_exited_ticket
+    if @ticket.exited?
+      redirect_to root_path, notice: '降車済みの切符です。'
+    end
+  end
+
   def ticket_create_params
     params.require(:ticket).permit(:fare, :entered_gate_id)
   end

モデルのコードについて

改札口モデルのコードは以下のとおりです。

exit?メソッドではまず、「同じ駅で降りようとしていたらfalseを返す」処理を行い、それから運賃を計算して切符の運賃と大小比較しています。

  def exit?(ticket)
-    true
+    return false if ticket.entered_gate == self
+    ticket.fare >= calc_fare(ticket)
+  end
+
+  private
+
+  def calc_fare(ticket)
+    from = ticket.entered_gate.station_number
+    to = station_number
+    distance = (to - from).abs
+    FARES[distance - 1]
   end

続いて、切符モデルのコードも見てみましょう。

切符モデルでは独自の検証メソッド（gate_exit_should_be_successful）を定義して、仕様上おかしなデータが保存されないようにチェックしています。

また、exited?メソッドは「使用済みかどうか（降車済みかどうか）」を返すインスタンスメソッドです。

+  validate :gate_exit_should_be_successful, if: -> { exited_gate.present? }
+
+  def exited?
+    exited_gate.present?
+  end
+
+  private
+
+  def gate_exit_should_be_successful
+    unless exited_gate.exit?(self)
+      errors.add(:exited_gate, 'では降車できません。')
+    end
+  end