フリーランス歴20年の強強エンジニアからのガチコードレビュー集

はじめに

こんにちは、まつけんです。
早いもので、Webエンジニアになって、10ヶ月経とうとしています。
先月末、僕の職場に参画していたフリーランス歴20年の強強エンジニアCさんが卒業されました。（以降Cさんと称します）

Cさんには、いつも迅速かつ丁寧なレビューをしていただいてました。
たまに補助で僕のプルリクにコミットを積んでもらうことなどもあり、お世話になった記憶が大半です。

今回はそんなCさんから受けたコードレビューから、今後どう改善していくのかアウトプットして学びを深めたいため、こちらの記事を書きました。

ペアプロしている時の参考になったこともおまけで書いてます。

この記事が参考になりそうな読者

• Rubyの「アンチパターン」から良いコードを学びたい人
• エンジニア1年目がどんなコードレビューを受けているのか知りたい人

※こちらの記事に出てくるコードに関しては全てRubyです。実務で学んだことなので、出てくるコードは全てフィクションです。(実際のサービスのコードではないです)

レビュー1: migrationファイルを追加する時「db:rollback」も考慮してね

〜〜〜〜migrationファイルを作成して、DBをいじるタスクの時〜〜〜〜

Cさん　「migrationファイルを追加するときは必ずrails db rollbackコマンドで更新前の状態に戻れるのか？　の確認をお願いします。
万一何らかのトラブルがあったときに、元に戻せない・・といった状況はなるべく避けたいです。
upの書き方をするならば、downも必要そうです」

僕　「その視点はなかったあああ。。。migration実行できて、満足してました。。」

[Before] アンチパターン

▼ 僕がレビューしてもらったmigrationファイルは以下のようにupの記述しかなく、マイグレーションの実行はできるけど、db:rollbackでマイグレーション実行前の状態に戻せないようになってました。

class <クラス名> < ActiveRecord::Migration[7.0]
  def up
    <マイグレーション実行時の処理>
  end
end

[After]　どう改善したか？

▼ downメソッドを追加することで、upメソッドで行った変更を元に戻す処理を定義しました。
これによって、db:rollbackでマイグレーション実行前の状態に戻せるようになりました。

class <クラス名> < ActiveRecord::Migration[7.0]
  def up
    <マイグレーション実行時の処理>
  end

  def down
    <ロールバック実行時の処理>
  end
end

最近で1番勉強になりました。そしてこの記事で、1番伝えたかったので最初に書きました。

学んだこと

• migrationファイルを追加したとき必ずrails db:rollbackでマイグレーション実行前の状態に戻せるかを確認する

参考記事

• 以下の記事がロールバックの処理について詳しく書かれてます

レビュー2: Viewファイルはシンプルに！ロジックは書かない方がいいよ

〜〜〜〜ビューファイルにゴリゴリロジックを書き連ねてしまった時〜〜〜〜

Cさん　「Viewファイルの中の〇〇の条件分岐はヘルパーの中で１つのメソッドにまとめてほしいです。
ヘルパーフォルダの中にViewで使いたいメソッドがまとまっています。
Viewファイルの中で 長いif文を書くよりも、ruby側でまとまっているほうがプログラムが書きやすいためです」

自分　「何となくイケてないのはわかってたけど、詳細なイメージが沸いてなかったので、ありがたいです！」

[Before] アンチパターン

▼ ビューにロジックをづらづらと書いてました。（実際のコードは11行くらいありました）

Viewファイル

- if product.price > 10000
  p 高価な商品です
- elsif product.price > 5000
  p そこそこ高価な商品です
- else
  p 手ごろな価格の商品です

[After]　どう改善したか？

▼ ロジックをヘルパーファイルに切り出し、Viewファイルを1行にすることができました

Viewファイル

p = price_category(product)

Product_Helper

module ProductHelper
  def price_category(product)
    if product.price > 10000
      "高価な商品です"
    elsif product.price > 5000
      "そこそこ高価な商品です"
    else
      "手ごろな価格の商品です"
    end
  end
end

学んだこと

• Viewファイルにはロジックをできるだけ書かない。できるだけヘルパーやモデルに切り出す

レビュー3: .include?ではなく==を使ったほうがいいかも

〜〜〜if文でinclude?を使って、条件分岐してた時〜〜〜

Cさん　「これでも動くのですが、1つ提案です。
.include? は、配列の中に対象の文字列が含まれているか？　を返すときに使われるのが一般的です。
この場合、変数 A は配列ではなく文字列なので、単に == で比較したほうが適切かもしれません。」

僕「変数の中に文字列が含まれているかを調べるときinclude？メソッド、脳死で使ってたかもです。。。」

[Before]

レビューで指摘されたコードは、正しく動く状態でした。ですが、変数Aが文字列（String型）だったので、「配列の中に指定した要素が含まれるか」を検証するためのinclude?は正しい使い方ではありませんでした。

if A.include?('specific_string')

[After]　どう改善したか？

変更後のコードでは、.include?ではなく==を使うことで、文字列の比較をして、true or falseを返します。

この変更で、コードの意図をより明確にし、可読性を上げることができたかな〜と思います。

if A == 'specific_string'

学んだこと

• include？メソッドは配列中に指定した要素が含まれているかを検証するために使う

参考

レビュー4: .min使ったら、5行を1行にできますよ

〜〜〜ある数値を超えると条件が切り替わる分岐を作ってた時〜〜〜

Cさん「この条件分岐、.min使ったら1行にできると思います」
僕「1行？　それはやるべきですね。やり方ググります。」

[Before]

(ruby_start..ruby_end).countが「10を超えるか超えないか」で条件分岐をしていました。
リファクタリングできそうでできないなあと思ってましたが、、、、

if (ruby_start..ruby_end).count > 10
   abc = 10
elsif (ruby_start..ruby_end).count <= 10
   abc = (ruby_start..ruby_end).count
end

[After] 　どう改善したか？

リファクタリングで1行になりました。
「配列の中の最小の要素を返す」.minメソッドを使うことで、1行にできました。

abc = [(ruby_start..ruby_end).count, 10].min

minメソッドを使うのは思いつきませんでした。。。if~endの【Before】のコードを見るだけでminメソッドを使って1行にできるかもと思いつくのは本当に流石だなあと。。。

学んだこと

• if~endの条件分岐をリファクタリングするときは、どうif~endを短くできるかだけに囚われすぎないこと
  • Rubyの他のメソッドを使えばif~endをまるっと省略できる選択肢もあるということ

参考

ペアプロ中1: 「git addする時、いつもどうやってますか？」

〜〜〜Cさんとペアプロ中〜〜〜

Cさん　『git addする時、いつもどうやってますか？　
　git add .よりも　git add <変更したファイル名>とする方が、不要な差分とかが入ってこないので、良いですよ〜』

僕　「ホントだわ。git add .は全てのファイルをインデックスにあげちゃうもんな。。。」

[Before]

Cさんからこの指摘を受ける前は、以下の流れでプッシュしてレビューを受けることが多かったです。

git add .
git commit -m 'コミットメッセージの内容'
git push origin HEAD

git add .で全てのファイルをインデックスに上げると、たまに意図せず変更した不要な差分が入ってしまうことがあり、Cさんから「不要な差分が入っちゃってます」と指摘されることが割とありました。

[After] 　どう改善したか？

Cさんの指摘を受け、自分がインデックスにファイルをあげる前（addする前に）git　statusで変更ファイルを確認して、プッシュし、レビューを受けるようにしています

git status 
git add <変更したファイル名>
git commit -m 'コミットメッセージの内容'
git push origin HEAD

レビューを受け、重要と思ったこと

自分が完璧だと思ってレビューを受けても、リファクタリングが足りてなかったり、想定する場面が考慮漏れしていたり等々、まだまだ実力不足だなと感じることがありました。

そして、1番大事と感じたのがプルリクを出す前の「セルフレビュー」です。
Cさんみたく門番のようなレビュワーの方がいると、セルフレビューがおろそかになることもあり、レビュワーの方の負担軽減をするため、今後は下記のように「セルフレビュー」により力を入れたいなと思ってます。

• ローカルで動作確認2周
• 「Files changed」で不要な差分が入ってないか確認
• リファクタリングできそうなところは事前にプルリクに書く

「セルフレビュー大事だよな〜」と思ってた中で、流れてきた最近のこのツイートがめっちゃ刺さり、また見返したいので記事に載せます。経験年数が浅い人は、100回はこのツイートを振り返るべき。

終わりに

Cさんからは本当に学ぶことが多くて、正直この記事に書ききれませんでした。
コードレビューを受けた以外の面でも学ぶことが多かったです。
▼例えば、、、

• 新機能開発の時、チーム全体での進捗共有を円滑にするためのシートを作る
• レビュワーがレビューしやすいように、プルリクの説明欄を動作確認の方法まで丁寧に書く　
  • プルリクの変更箇所を画像付きで書く

他にも色々。。。。

そして、「仕事は楽しむもの」と言ってたのが印象的です。
僕自身、たまに成長することに囚われすぎて、少しナーバスになることがあるので、仕事を楽しむことを忘れないようにしたいなと書きながら思いました。

エンジニアとしての駆け出し期に、模範となる働き方を背中で見せてくれたCさんと働けたのは僕の財産です。