DDD ディストーション駆動開発

エレキギターと音楽とRubyを愛するフルテンエンジニアのblog

Rails Developers Meetup #4の公開コードレビューを受けてみて

Rails Developers Meetup #4

先週開催されたRailsDM #4、残念ながら会場参加はできなかったのですが、twitterでずっと追っかけてました。

twitter.com

先日PullRequestした公開コードレビュー、イベント当日はコードチェックしてもらえなかったのですが、なんと翌日、レビューしていただくことができました!

全PRに対するレビュー動画

PR送った全員に、jnchitoさんが動画でコメントくださってます。すごい!! 

www.youtube.com

www.youtube.com

レビューのふりかえり

フィードバックは以下の通りでした。振り返ってみると、絶対やってはいけないことをがっつりやっちゃってて、非常に恥ずかしい。。。

不要なreturn

Answer by itume · Pull Request #17 · JunichiIto/train-ticket-rails · GitHub

ご指摘の通りですね。。。Rails始めたばっかりの時によく見たDoubleRenderErrorのトラウマ?でつい手癖で and returnしちゃうんですよね。全く気づいてなかったです。。。

降りられるかどうかの判定はGateがやるべきでは?

コメントいただいてる通り、現実世界で改札機と切符、どっちが判定してるのだろうか?って考えたら、そら改札機ですよね。なぜ切符の責務だと思ったのか。。。そのせいでGateとTicketを行ったり来たりしてて、気持ち悪いですね。。。

判定の責務を改札機にもたせて書き直したら、めっちゃスッキリ書けました。

Answer by itume · Pull Request #17 · JunichiIto/train-ticket-rails · GitHub

現実世界に存在しない概念をシステム内に作ると、大概失敗するんですよね。。。

やりきれない事に手を出しちゃいけない

これは今回の最大の反省点。

Arrayのindexに、index以上の意味をもたせてはいけない、という自分の経験上の鉄則があって、今回のコードのGate::FARESを見た時に、反射的に「これはあかんやつや」と思って、Hashで再定義しました。

Answer by itume · Pull Request #17 · JunichiIto/train-ticket-rails · GitHub

Hashならkeyが使えるので、Arrayのindexのようにうっかり変わっちゃったりせず、*1安心安心、、、とか思ってたんですが、今回触ってはいけない事になっているviewにGate::FARESがいるので、Gate::FARESを消すことはできなかったのです。

こうなると、ご指摘の通り、二重管理しないといけなくなっちゃうのですよね。悪夢ですね。

Gate::FARESを消せない、とわかった時点で、ここに手を出すべきじゃなかった、ということです。やり遂げられないリファクタリングはやっちゃいけないです。*2

公開コードレビューに参加してみて

参加して本当に良かったです。色々感じることがありました。

実際に自分でコードを書いて、人の回答とレビューをみたり、自分のコードに対するレビューを見るのと、当日のスライドやレビュー動画だけ見るのでは全然情報量が違ってくるんじゃないかなあ、と思います。

身の回りのシステムのロジックをぼんやり考えちゃう 

降りられるかどうかの判定はGateとTicketどっちの責務?って考えてから、身の回りのシステムってどうやって動いているんだろうか?ってぼんやり考えることが多くなりました。今まではwebのことばかり考えていましたが、世の中で動いてるいろんなシステムの設計について思いを巡らせるのも面白いですね。

思った以上にいろんなコードを書く人がいる

かなり自由度を狭めた中でのコーディングだったはずなのですが、蓋を開けたら十人十色で、一つとして同じ回答がないのには本当に驚きました。同じことやるメソッドの命名も人によってそれぞれだったり、commit messageの書き方もそれぞれですね。

中には「その発想はなかった(悪い意味で)」というコードもあったりしました。*3こういう課題をエンジニア採用時に出してみたら、その人のコーディングスタイルとか、物事の捉え方とかが見えて来るのでいいかもしれないですね。

結局FARESはどうしたらいいんだろうか

jnchitoさんもREADMEでコメントされていますが、Gateが持ってるべきではないような気はするのですよね。ただじゃあどうするよ?というと、なかなか難しいですね。

  • 定数だけのmoduleにする
  • プレーンなClassにする
  • modelのconcernにする
  • テーブル作ってActiveRecord::BaseのClassにする

といった手段が取れると思います。

一番しっくり来るのはテーブルにするパターンな気がしますが、たった2種類の料金のためにテーブル作るのは明らかにやりすぎですね。

アプリケーションの規模を考えると、一旦Gateに持ってもらう、は絶妙なさじ加減ですね。テストがあるから必要ならあとでなんとでもできますしね。

 

と、いう感じでふりかえりしつつ、コード修正してpushしたら、PR openのままだったから通知飛ばしてしまったみたいで。。。スイマセン。。。

 

参加してみて良い勉強になりました。

RailsDM運営の皆様、jnchitoさん、ありがとうございました。

rails-developers-meetup.connpass.com

*1:例えば160円の特別運賃ができた時、190の後ろに入れれないとindexが変わってテストが落ちるけど、順番に並べたくなっちゃう

*2:余談ですが仕事で触ってるコードは、志半ばで放置された「リファクタリング仕掛り中コード」がいくつかあり、多くが二重管理地獄と、忘れた時にやってくる地雷と化していてかなり辛い事になってます。。。そういうコードに限ってテスト書かれてないですしね。。。はあ。。。

*3:まあ自分のやつもそうですけどねorz