🦔

コードレビューの時にどこを見ているか

2025/01/30に公開

エンジニアとしての歴も少し積み重なってきたせいか、レビューをしているといくつか過去の経験が浮かんでくることが最近ちらほらありました。

覚えてるうちにそれらを言語化してみようかなと思います。

テストのないコードはapproveしない

「変更とテストは必ずセットにすること」。昔、大先輩からいただいた言葉です。

一方で「テスト書いてください」って言いづらいですよね。わかります。うざいやつだって思われたくないし、目上の人が相手なら尚更。

僕の場合は、この先輩が本当にすごい人だったというのが大きいかもしれません。ちゃんとやってる人ってやっぱちゃんとしたすごいコードを書くんですよね。

最近の言い方だと「勇者ヒンメルならそうした」でしょうか。
心にヒンメルを宿して、やっていきましょう。

(もちろんHotfixすぎて時間ないとか、既存のコードやテストデータのセットアップが複雑でテストを書くのが容易ではないといった例外はあります。そのような場合は実際に動かしてみたりQAでOKが出ればテスト無しでも良い)

「分岐」なので必ず2つのケースが存在する

例えば、「XXXの時 name が更新される」という仕様が加わったとします。
ここでそのケースだけをテスト追加するのは不十分で「XXXではない時 (に、どうなる) 」というテストケースも同時に必要かなと思いますが、結構よく忘れがち。
あるいは、「nameが更新される」の方に着目して「nameが更新されない」のはどういう時か?という視点で考えてみるのもいいと思います。

大抵の場合ある処理に対して、「成功」「失敗」または「何も変化しない」という状況があるというのを頭に置いてコードを読んでいくといいですね。

これについても、条件分岐が明らかに自明だったり似たような他のケースでカバーできている場合は厳密に見なくてもいいかなと。(Controllerのテストで毎回401のテストしなくてもいいよねみたいな)

期待値のカバレッジ

「CSVを読み込んで複数のレコードを一括更新する」みたいなの、よくある話かなと思います。これを更新された全レコードに対して厳密に検査するのは大変です。
一方で、複数のレコードがちゃんと全部処理されるか?も気になる。

なので、ある単一のレコードにはフィールド全てを見るような厳密なテストを行い、同時に流したその他複数のレコードは件数が合っているか、並び順が合っているか、といった観点だけをテストしたり、心配であればフィールドの更新をするメソッドをスタブしてそれが件数分呼ばれるか?というケースを作るのもいいですね。(ヨコのテストとタテのテスト)

またこの時値を検査するレコードには、ちゃんと全ての値を埋めてテストするというのも大事なとこです。nullableのフィールドに値が入ってなくてもテストがパスしていて、いざ値が入った場合に期待する挙動になっていなかったみたいなことがよくあります。

扱うものが大きいとついサボりがちですが、MECEにやっていくのが大事です。同時に、このようなテストの組み合わせでパスできるように実装の方も適切な責務の範囲に分離させていくべきだ、という感じでテストから逆算されて実装も綺麗になっていくと思います。

コードそのもののカバレッジが高いのはもちろん悪くないのですが、それよりも「よく使われている実装がどこまで厳密にテストされているか」 すなわち、コアドメインのユースケースや状態に対するカバレッジ の方が大事なんじゃないか?と最近は思っています。

説明と結果が一致しているか

テストケースの説明 (context, it, expected) と、実際に検査される値・期待値が一致していないというのも割とよく見かけます。例えば、「it 'ABCの tel が一致する' 」と書いているが、実際の挙動では tel ではなく id との一致を見ているみたいなやつです。

大抵はtypoやコピペして修正漏れだったりするんですが、たまに説明と結果が逆じゃない?みたいなこともあったりします。
こういう時、仕様の説明や実装がわかりやすければすぐ気付きますが、ちょっと複雑になると「実装とテスト、一体どっちが正しいのか?」という話になってきます。

時間が経過して実装した当時のメンバーがもういないとかなってしまうと更に厄介で、いざ変更しようとした時、いちいち調べるのに余計な時間を使ったりします。(保守性の低下)

シンプルに名付ける

誰でもやったことがあると思います。
「短くするために意味を省くぐらいなら、多少長くても適切な単語を充てる」
わかります。僕も基本のスタンスはそうです。

けどやっぱ物事には限度があります。どこかで諦めて切っていかないとコード全体が間延びしまくって読みづらくなるという本末転倒なことが起きてしまう。

アプローチはいくつかありますが、まずはよりシンプルな語を充てれないか検討する。
それも難しそうならいっそ略語にしてしまう。

(例: UserManagementOfficeRelatedCompnay -> UmoRelatedCompany
※UserManagementOfficeが事業ドメインとして頻繁に登場する場合はこのやり方もあり)

あるいは、NameSpaceが使えるならそれも良いアプローチです。
UserManagementOffice::RelatedCompany
User::ManagementOffice::RelatedCompany

「嫌な予感」は結構当たる

ぱっと見良さそうなんだけどなんか違和感あるなー、と思った時。意外とその予感は当たっている気がします。
とりあえずその違和感をGPTとかパープレにざっくり聞いてみましょう。いい時代になりましたね。

違和感を感じたらまず何か動かしてみる、調べる。「あの人が実装してるんだからまあ大丈夫だろう」というのはまあまあ死亡フラグです。

(経験した例で言うと、引数が足りなくて更新できてなかった・例外を発生させるメソッドの書き方ができていると思っていたが実際には起こらずロールバックされなかった・upsertの挙動と思っていたが実際にはcreateだった… などなど。外部サービス依存でテストが書きづらい ⇒ 意図する環境での動作検証が不十分 みたいな時にこういうのがよく起こります。普段あんま触らないから「違和感」ぐらいでしか検知できなかったり)

まとめ

自分で書いたものを見返してみて、「だいぶ具体的な話に寄ってるな」と思いました。これはおそらく、コードレビューという具体的なものが仕上がった段階で行われる作業のせいかなと。

レビューの段階であれこれ言われるのって気持ちの面ではあんまよくないですよね。
仕上がった料理を持ってきたものに対し「あかんでこれ」と言うのは、お互いにしんどいコミュニケーションになるのは自明です。が、やはりダメなものにはダメと言わなければいけないし、レビューされた側は言われたことにちゃんと応答する義務はあると思います。
もちろん、レビュープロセスを通じて良いコードが生まれる・別な観点が発見されるなど自身の経験としてプラスになる部分はたくさんありますが、できるなら最初にそれができた方がいい。

レビューであれこれ言わなくて済むように、ここから立ち返って設計や実装のプロセスが改善されるような仕組みを整えていくことが大事なのかなと改めて思いました。(できるとは言ってない笑)

読んでいただきありがとうございました。

アルダグラム Tech Blog

Discussion