最近コードレビューで指摘したこと

1. 最近コードレビューで 　　指摘したこと 1 株式会社ラクーンホールディングス 技術戦略部 羽山純
2. 自己紹介 • 名前 ◦ 羽山 純（Jun Hayama） ◦ 　　@forrep • 所属 ◦ 株式会社ラクーンホールディングス 技術戦略部 • 技術領域 ◦ バックエンド・インフラ ◦ パフォーマンス改善 ◦ AI（企業審査AI） • 個人活動 ◦ アプリ開発 2
3. 後でコードを読む人の消費時間を最小化する 3
4. 後でコードを読む人の消費時間を最小化する • 次のパターンは正常系に進む ◦ form.hasErrors() == false かつ firstError != null ◦ form.hasErrors() == true かつ firstError == null • 後から読む人は上記パターンの調査が必要になる ◦ あえてそういうコードとなっている、無視できない public String controller(Form form) { if (form.hasErrors()) { Error firstError = form.getFirstError(); if (firstError != null) { return firstError.getMessage() } } // 正常系の処理 // ... } 4
5. 後でコードを読む人の消費時間を最小化する • 必要のない条件は削除する、でないと後の人が困る • 必要な条件なら、再現できるテストケースを書く public String controller(Form form) { Error firstError = form.getFirstError(); if (firstError != null) { return firstError.getMessage() } // 正常系の処理 // ... } 5
6. 要件外/想定外の責任をしっかり放棄する 6
7. 要件外/想定外の責任をしっかり放棄する • 赤枠のフィルタ処理は必要なさそう • from.getErrors() はフレームワーク内の処理で不透明 ◦ null 要素が混じる可能性はゼロではない（ほぼゼロだけど） • 後で修正する人は、この行を簡単に削除できない ◦ この状態で数年間動いてたら、消すのは怖い ◦ 必要性は分からないが、修正後も維持するしかない ◦ null 要素を含むテストは不可能でモヤモヤする Optional<Error> firstError = form .getErrors() // Stream<Error> .filter(Objects::nonNull) // Stream<Error> .findFirst(); // Optional<Error> 7
8. 要件外/想定外の責任をしっかり放棄する • 要件外/想定外の事象には対処しない ◦ 想定しない null 要素は致命的な問題の可能性 ◦ 動作を続けるよりもフェイルファストで落とす方が安全 ▪ 動き続けたら問題の発生自体に気づけない • 想定外へ対処するとあなたは安心するが後の人は不安になる ◦ 責任をしっかり放棄することが、責任を取るということ ▪ この状態でしばらく正常稼働すれば問題ないと確認できる ◦ 「想定しない状態だけど動くように」は害でしかない Optional<Error> firstError = form .getErrors() // Stream<Error> .findFirst(); // Optional<Error> 8