はじめに
2017年8月24日にRails Developers Meetup #4という勉強会で「プロを目指すRailsエンジニアのための公開コードレビュー」という発表をしてきました。
このエントリではこの勉強会の発表内容を紹介します。
発表のテーマを決めるまでの経緯
以前、「WEB+DB PRESS Vol.99の「良いコード」を本気でコードレビューしてみた」というエントリを書いて結構な反響があったのですが、この記事を読まれた主催者の平野さん（@yoshi_hirano）から「良いコードとは何か、というテーマで体系的に語ってもらいたい」と声をかけてもらったのが登壇のきっかけでした。
ただ、「良いコードを体系的に語る」となると、僕の中ではおそらく「CODE COMPLETEやリーダブルコードを読め。以上！」になってしまうので、それよりも具体的なコード例を見ながら「ここはこう」「このコードはこう」と説明する発表の方がいいだろうなと思いました。
また、具体的なコードを見ながら良し悪しを語るにしても「お互いに知っているコード」でないと、なかなかぱっと理解することができません。
そこで、
- 僕の方で簡単なプログラミング問題を作る
- それをみんなに解いてもらう
- さらに、いただいた解答をいくつかピックアップして発表の中でコードレビューする
というスタイルにするのはどうか、と提案しました。
平野さんも「いいですね。面白そう！」と言ってくれたので、ちょっと珍しい「公開コードレビュー」という形式の発表をやることになりました。
CODE COMPLETE 第2版 上 完全なプログラミングを目指して
- 作者: スティーブマコネル,Steve McConnell,クイープ
- 出版社/メーカー: 日経BP社
- 発売日: 2005/03/26
- メディア: 単行本
- 購入: 44人 クリック: 1,166回
- この商品を含むブログ (288件) を見る
リーダブルコード ―より良いコードを書くためのシンプルで実践的なテクニック (Theory in practice)
- 作者: Dustin Boswell,Trevor Foucher,須藤功平,角征典
- 出版社/メーカー: オライリージャパン
- 発売日: 2012/06/23
- メディア: 単行本（ソフトカバー）
- 購入: 68人 クリック: 1,802回
- この商品を含むブログ (137件) を見る
問題の概要
僕が作った問題はTrainTicketRailsという、電車の改札口を簡易的にシミュレートしたRailsアプリです。
あらかじめある程度コードができていて、そこに指定された仕様を満たすコードを書いてもらう、というのが今回のお題になります。
詳しくは以前書いたこちらのエントリをご覧ください。
ちなみにこの問題は2017年11月に発売予定の書籍「プロを目指す人のためのRuby入門」のために作った例題をRails向けにモディファイしたものです。
当日は自宅からリモート登壇！
この勉強会はなんと、僕は自宅から発表する「リモート登壇者」でした。
さらに、登壇者だけでなく、勉強会の会場もメイン会場の東京会場に加えて、東京会場と中継でつなく大阪会場と、自宅から視聴可能なリモート会場もありました。
発表する側も、発表を聞く側も、リモートでつながる勉強会とは何とも未来的ですね～。
ちなみに当日は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
さらに：全員分のコードレビューもしてみました！
もともとはこちらでピックアップした2名のコードをレビューして終わり、とする予定でした。
ですが、他のみなさんのコードも非常に興味深いものが多かったので、思い切って全員分レビューしてみることにしました！
勉強会の時と同じようにpull requestを見ながら口頭でコメントし、その様子を動画に撮ってYouTubeで公開しています。
ちょっと長いので前編と後編の2部構成になっています。
本編では話していない話題もたくさん出てくるので、こちらもぜひチェックしてみてください！
（ただし、長いので1.5倍速ぐらいで視聴することをオススメします）
Twitter上の反響
Twitterの反響を見ていると思った以上に楽しんでいただけたようで、登壇者としても非常に嬉しかったです。
勉強になりました！ > 公開コードレビュー #railsdm
— is_null() (@NAKANO_Akihito) 2017年8月24日
予想以上に納得感があった #railsdm https://t.co/hf9Qy0FXF9
— Leon (@Leon_O_Neal) 2017年8月24日
伊藤さん流石だったなー。「意図が分かるロジック」よりも「意図が分かる名前」をにかなり共感。 #railsdm
— 疋田 駿 (@shunhikita) 2017年8月24日
勉強会全体のツイートをご覧になる場合はこちらのTogetterをどうぞ。
僕が発表した時間帯のツイートは3ページ目から9ページ目あたりにあります。
まとめ
というわけでこのエントリではRails Developers Meetup #4で発表した「プロを目指すRailsエンジニアのための公開コードレビュー」の内容をあれこれ書いてみました。
主催者の平野さんをはじめ、運営スタッフのみなさん、どうもありがとうございました。 入念な準備のおかげでスムーズに発表することができました！
また、公開コードレビューで解答をピックアップさせてもらったお二人をはじめ、解答を投稿してくれたみなさんもどうもありがとうございました！
解答が来なかったらどうしよう？と心配していたのですが、解答を投稿してもらったおかげでとても実りのあるコードレビューができたと思います。
これからRubyやRailsの勉強をやっていこうと思われている方は、ぜひ今回の発表内容を参考にしてみてください。
あと、2017年11月に発売される「プロを目指す人のためのRuby入門」も良いコードを書くのに役立つと思うので、こちらもぜひよろしくお願いします！（宣伝）
告知：Rubyプログラミングキャンプ2017、参加者募集中です！
僕とAkiさん（@spring_aki）で主催している西脇.rb＆神戸.rbでは現在「Rubyプログラミングキャンプ 2017」の参加者を募集中です！
このイベントは2日間、泊まり込みで思いっきりRubyプログラミングに没頭しようという楽しいイベントです。
まだ若干枠が残っているので、興味がある方はぜひご参加ください。
Ruby初心者の方や、僕たちの勉強会に一度も参加したことがない、という方も大歓迎です。
もちろん僕も参加するので、「伊藤さんに良いコードの書き方を教わってみたい」という方もぜひ！（苦笑）
nishiwaki-koberb.doorkeeper.jp
あわせて読みたい
今回登壇するきっかけになったコードレビューのエントリです。
自分で読み返してもかなり細かいな・・・。
僕のコードレビューが細かくて、良いコードや良い設計へのこだわりが強いのはこんな背景があるからです。
