Ruby/Railsのコードレビューで指摘されやすいポイント
概要
Ruby/Railsのコードレビューで指摘されやすいポイントをまとめました。 セルフレビューでも意識できるように、定期的にまとめておこうと思います。
ActiveRecordでクエリを書く時
バッチ処理など、少し複雑なクエリをARで書く時に、パフォーマンスなどの面で指摘されがちな点です。 普段分析業務などで、BigQueryなどを使っているとパフォーマンスに対する意識が薄れやすいので気をつける必要があります。
使うカラムだけ取得しているか？
Model.where(...)ではなく、Model.where(...).select(:some_column)とする。Databaseへのアクセスは何回発生するか？必要最低限になっているか？ 例えば、
pluckをしたタイミングでto_aが走るため、本当に必要でなければ、select(ActiveRecordのメソッド)で必要なカラムの情報だけ取得して、次の条件の絞り込みに使うなど(SQLを組み立てる段階では、サブクエリとして用いられる)不必要なオブジェクトの生成をしていないか？ ActiveRecordのオブジェクトは割とサイズが大きいので避ける。
データサイズより、ネットワーク越しのリクエスト回数(Databaseへのアクセス回数)に注意
エラー処理に関して
raise error がされたらプログラムが終了するので、巨大なバッチ処理などの場合などはどこかのレイヤーでrescueしてあげて、適切な処理をしてあげる必要がある
rescue で全てのエラーをrescueしてしまうのは危険
重要なエラーさえ握りつぶしてしまう可能性があるので、想定しているエラーのみをrescueするような形にする
ensure はエラー発生の有無に関わらず実行される
! がつくメソッドは二つの意味を持つので注意
破壊的メソッド
- エラーが返され売る時(create!, save!とか)
その他一般的なこと
文言周りに関しては基本的にはi18nを使うようにする 後々の言語対応をしやすくする
リンクを渡すときは基本的にヘルパーで タイポを防ぐ、後々変更しやすくする というメリットがある
バッチ処理を打つ際のエラー処理
ループなどでエラー処理をする際は具体的にどのオブジェクトでエラーが起きたのか？まで取得しておくと、後々デバッグがしやすい
まとめ
指摘されたポイントを振り返ると
- パフォーマンス
- 後から読んだ時意味がわかるようにしておく
- 変更しやすい, 壊れにくい
- トラブル時の対応しやすさ
などの観点が挙げられそうです。 実際にトラブル対応した経験や、レガシーなコードで困った経験などをしたからこそ、持てる観点だと思うので、同じ思いはしない(人にもさせない)ように、気をつけていきたいですね！
