コガミツlog

福岡在住エンジニアのブログ

コードレビューで学ぶRuby on Rails を読んで新しく得た知見

techbookfest.org を読んだので新しく得た知見を書いておきます。

読むきっかけ

  • たまたまTwitterで見かけた
  • PRをレビューする機会は今も未来もあるので新たなレビューの視点が欲しかった
  • 自分が実装する時でも役に立つものがありそう

勉強になったところ

テストコード単体で期待値の根拠を示す

factoryでデータの定義を見に行かないとテストコードの正しさがわからない問題に対して、 期待値に使う値は同じテストコード内で準備する。という提案が書いてありました。

例えば、Bookがtitleという属性を持っていて、titleが"たのしいRuby"になっていることを期待するテストの場合、 factoryでtitleのデフォルト値を"たのしいRuby"と記述していた場合、FactoryBot.create :bookでデータを用意するとRSpecのファイルだけだとtitleが"たのしいRuby"になっている理由がわからず、factoryの定義を見に行く必要が出てきてしまう。

されないことの検証には注意する

例えば、ある属性を持つデータは一覧画面に表示されないことを期待するテストの時。 表示されない条件は多い。

  • visitに間違ったパスを指定していた
  • 画面がホワイトアウトしていた
  • 検証するデータにtypoがあり、期待値とは違うデータは表示されていた

提案として

表示される条件と一緒に確認する。 そうすると、少なくとも遷移先の誤りや画面のホワイトアウトには気づける。

テスト書くことの工夫として、一度失敗した上で成功させるように書くと良い。

Railsにおける命名

意識せずとも自然とできていましたが言語化ちゃんと出来ていない部分もあったのでメモ。

クラス

  • 名詞をつける
    • Rubyはオブジェクト指向型スクリプト言語なので、オブジェクトはモノなので名詞の方が良い
  • 動詞とかだと違和感あるので名詞をつけていたと思う

メソッド

  • 副作用のない、返り値を期待しているなら名詞
  • 副作用があれば動詞をつける
  • あとはRubyっぽく、?や!を使う
  • 多分出来てたと思うけど、英語の単語的に間違って動詞を使っていたケースがあったかも。

Hash

  • xxx_map, xxx_paramsが多い
  • なるべき限定したスコープで使う
    • どのような情報が格納されているか分かりにくいため。
    • 言われてみると確かにhashを色んな箇所で使い回しているコードはあまり見たことがないです

禁止リストより許可リストを優先的に使う

skip_before_action :authenticate_user!, except: %i[create]

みたいなコードにおいて、新しくアクションを追加した時に、exceptの対象に入れてしまうと認証を通さなくてアクションを実行できることになる。

許可リストの方が情報漏洩など大きな問題になりにくい。