最近コードレビューで 　　指摘したこと 1 株式会社ラクーンホールディングス 技術戦略部 羽山純

自己紹介 ● 名前 ○ 羽山 純（Jun Hayama） ○ 　　@forrep ● 所属 ○ 株式会社ラクーンホールディングス 技術戦略部 ● 技術領域 ○ バックエンド・インフラ ○ パフォーマンス改善 ○ AI（企業審査AI） ● 個人活動 ○ アプリ開発 2

後でコードを読む人の消費時間を最小化する 3

後でコードを読む人の消費時間を最小化する ● 次のパターンは正常系に進む ○ 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

後でコードを読む人の消費時間を最小化する ● 必要のない条件は削除する、でないと後の人が困る ● 必要な条件なら、再現できるテストケースを書く public String controller(Form form) { Error firstError = form.getFirstError(); if (firstError != null) { return firstError.getMessage() } // 正常系の処理 // ... } 5

要件外/想定外の責任をしっかり放棄する 6

要件外/想定外の責任をしっかり放棄する ● 赤枠のフィルタ処理は必要なさそう ● from.getErrors() はフレームワーク内の処理で不透明 ○ null 要素が混じる可能性はゼロではない（ほぼゼロだけど） ● 後で修正する人は、この行を簡単に削除できない ○ この状態で数年間動いてたら、消すのは怖い ○ 必要性は分からないが、修正後も維持するしかない ○ null 要素を含むテストは不可能でモヤモヤする Optional firstError = form .getErrors() // Stream .filter(Objects::nonNull) // Stream .findFirst(); // Optional 7

要件外/想定外の責任をしっかり放棄する ● 要件外/想定外の事象には対処しない ○ 想定しない null 要素は致命的な問題の可能性 ○ 動作を続けるよりもフェイルファストで落とす方が安全 ■ 動き続けたら問題の発生自体に気づけない ● 想定外へ対処するとあなたは安心するが後の人は不安になる ○ 責任をしっかり放棄することが、責任を取るということ ■ この状態でしばらく正常稼働すれば問題ないと確認できる ○ 「想定しない状態だけど動くように」は害でしかない Optional firstError = form .getErrors() // Stream .findFirst(); // Optional 8