#book #r2021 ![image](https://gyazo.com/ff60492918505ee8cf27f3bab2d82acb/thumb/1000) 発表版もある [https://youtu.be/jXi8h44cbQA](https://youtu.be/jXi8h44cbQA) スライド [https://speakerdeck.com/trishagee/code-review-best-practice](https://speakerdeck.com/trishagee/code-review-best-practice) # なぜこの本を読んだか [https://techleadjournal.dev/episodes/10/](https://techleadjournal.dev/episodes/10/) の podcast を聞いてて気になった Code Review のガイドとしては、 [https://google.github.io/eng-practices/](https://google.github.io/eng-practices/) も好きだけどもう一つ読んでおこうと思った この本は、Jetbrains で Developer Advocate をしていて Java Champion でもある Trisha Gee さんによる本 [https://leanpub.com/whattolookforinacodereview](https://leanpub.com/whattolookforinacodereview) でミニマム無料で手に入る # 何が書かれている本か Code Review のどういう観点をなんで見るべきかをまとめている。 特に他の本やブログではあんまり言及されないような、データ構造やテストの保守性やパフォーマンスの問題に触れている点で新鮮な内容になっている。 # メモ ## What should you look in general Design - コードが既存のアーキテクチャに fit しているか - SOLID原則などの原則を守れているか - コードに利用されているデザインパターンは適切か - コードはチームのコードスタンダードを満たしているか - クラスなどの配置場所は適切か - 既存のコードを再利用できるか?新規のコード自体に再利用性はあるか - over-engineered になっていないか、YAGNIなどは守れているか Readability & Maintainability - 命名は表現したいことを適切に表現できているか - コードを読んで何をしているか理解できるか - テストが何をテストしているか理解できるか - テストはケースを適切に満たしているか。正常系の他に異常系もカバーできているか。 - Exception や Error のメッセージは理解しやすいか - 複雑なコードは、ドキュメンテーションやコメントやテストなどで補足の説明が適切に行われているか Functionallity - コードは要求を満たしているか。テストがある場合には、テストが要求を正しくテストできているか。 - コードは、微妙なバグなどを含んでいないか (and と or の誤りや check する変数の誤りなど) Have you though about...? - セキュリティの問題がないか - 何かしらの規制などを破るようなことはないか - パフォーマンスの問題などを生じないか - ドキュメントを新規に作成する必要がないか、また既存のドキュメントを編集する必要がないか - ユーザーに見えるメッセージは問題ないか - 本番環境で停止してしまうことはないか - 本番環境でテスト環境を見てしまうようなことはないか さらにあまり話題に出ないような領域を深掘っていく ## Tests ### Are there tests for this new/amended code? ### Do the tests at least cover confusing or complicated sections of code - 複雑なコードについて少なくとも1つのテストがあるかは重要 ### Can I understand the tests - 読みやすいというのもそうだし、保守を前提に変更に対して柔軟である必要がある ### Do the tests match the requirements? - 要求を満たすテストは最低限作る - 異常系など失敗した場合のテストも当然かく ### Can I think of cases that are not covered by the existing tests? - レビュアーは、テストやユーザーストーリーの受け入れ条件にはないエッジケースを考えるべき ### Are there tests to document the limitations of the code? - 制限がある場合にはその制限がテストとして記述されるべき - 制限を表明するためのドキュメンテーションの役割も担う ### Are the tests in the code review the right type/level - テストピラミッドなどの観点でテストが正しいレベルかどうか ### Are there tests for secruity aspects? - 例えば、ログインのテストを書くときにはログインしていない状態でどうなるかのテストも書くべき ### Performance Tests ### Reviewers can write test too - レビュアーもテストを書くことはできるので suggestion やペアプロを活用してみる - レビュアーは、コードのレビューだけじゃなくてなるべく実行してみて機能を触ってエッジケースなどを見つけるのも有効 - この際にテストを新規に書いて実行して、そのテストコードを suggest するのもよい ## Performance Perforamnce Requirements ### Does this piece of functionality have hard performance requirements? ### Has the fix/new functionallity negatively impaceted the results of any existing performance tests? ### What if there are no hard perfomance requirements for this code review? Calls outside of the service/application are expensive ### Calls to the database - 本物のDBにトランザクションを実行するのはコードを単純に処理するよりも遅い - N+1 問題なども同じくパフォーマンスを悪化させる要因 ### Unnecessary network calls ### Mobile / wearable apps calling the back end too much Using resources efficiently and effectively ### Is there something in the code which could lead to a memory leak? ### Is there possibility the memory footprint of the application could grow infinitiely? - 徐々にメモリが圧迫されるようなことがないか - 永遠に容量が増えていってしまうような変数がないか ### Does the code close connections/streams? ### Are resource pools correctly configured? Warning signs a reviewer can easily spot ### Reflections - 特に Java ではリフレクションは遅いというのはよく知られた事実である ### Timeouts - 最悪のケースとして適切なタイムアウトが設定されているか ### Parallelism Correctness 主にマルチスレッド環境でも問題なく動くか ### Is the code using the correct data structure for multi-threaded environment? ### Is the code susceptible to race conditions? ### Is the code using locks correctly? ### Is the performance test for the code valuable? Caching - キャッシュの状態管理はうまくできているか ## Data Structures ### Lists - 順番を重要視しているデータ構造 - Key による取り出しが多かったり、ソートが多く発生する場合には別のデータ構造を使うのがいい ### Maps - Key-Value 形式で Key を起点に値を取得する場合のデータ構造 (そしてこの計算量が O(1) であることでも知られている) ### Sets - 重複した値を許容しないデータ構造 - 順番でアクセスしたり for でアクセスするには向いてない ### Stacks - Last In First Out (LIFO) のデータ構造 ### Queues - First In First Out (FIFO) のデータ構造 ### Why Select the right data structure? - Perfomance - Stating Expected Behaviour - 型と同じである程度 Data Structure によって想像しながらコードを読むことが多い - Reducing Complexity - 最小限の複雑度でやりたいことを実現することが重要 # 感想 - コードレビューで見るべきもので自分が普段見ているものと似ていたのでよかった - podcast の中では、 Design は diff でのレビューは難しいから diff が出来上がる前に話すべきということが言われててそれも触れてあるとよかった - データ構造とかについては指摘する人も少ない印象なので触れていてよかった - テストについても割と重要視して書いてあって参考になった - Java に特化したものも多かった (著者のスキルがJavaよりだから) ので全ての言語で同様にっていうわけではなさそう