# レビューに対する観点をまとめようのドキュメント ## 目的 前回の振り返りで、「mergeのタイミングが難しい」(どうしたらmergeして良いのかがわかりづらい)という話が出ました。 こちらに対して、「レビューの目的を明確にしよう」ということになりました。 この目的を決めるにあたり、今までどのような観点を大切にしてレビューをしてきたのか、またはレビューを期待してきたのかを収集して、そこから決めよう、という話になりました。 ## やること みなさんそれぞれのレビューの観点を記載してください。 ## 期限 次の定例(10/29(火))まで ## ここから記載欄 ### かいさん ### くがさん - プルリクエストの目的、背景 - 関数などの IO や 型 - 問題のありそうな記載があるかなど ### じまぐさん - コードスタイルが統一されているか - 既存のコードと同様の記述になっているか - 冗長な書き方をしていないか - 既存の定数やモジュールを使い忘れていないか - 名前付けが適切かどうか - 相応しい単語を選んでいるか - プロジェクト内で同一のものを別の名前で呼んでいないか - PR範囲外の箇所に影響を与えていないか - 言語的なバッドパターンを踏んでいないか - レビュアーの負担を減らそうとしているか - 機能・課題に対してPRで何を行ったかを記載してほしい - PRの影響範囲も ### たかみさん - 実装詳細の理解/把握に加え、経緯や状況の共有（なのでPRに要件とか経緯とかがある程度補足されているとありがたい） - 考慮漏れ、書き損じ、その他のミスを拾ってフォローする（してもらう） - もっといい書き方（可読性や保守性の高い書き方）を知っている人に教えてもらえる ### とりばさん - 考慮漏れの防止（不具合発生を未然に防ぐ） - より良い実装方法・記法などを追求することでコードの品質・パフォーマンスを持続させる ### まえださん 1. 他の人にコードを見てもらうことで、「この人しかそのコードを知らない」という状況を防ぐ(5割くらい) 1. 達成したいことに対するエッジケースの考慮漏れなどを防いで安全にする(3割くらい) 1. コードの可視性や保守性をより高める(2割くらい) ### まつもとさん - 機能外の統合的な考慮漏れが無いかどうかの確認 - 実装が属人的にならないように共有する ### みるこんさん * みんなにコードを読んでもらって, そのタスクの仕様や, 設計なんかを把握してもらう * コードに大きな欠陥がないか, 及びその他 code readability や, testability などのコードの保守性を確かめる。 * 特にレビューの際,以下のことを気をつけている。 * コードに致命的な欠陥がないかどうか (関数が使われてるのにインポートが漏れてるとか) * (要件を把握している場合のみ) そのコードが要件を達成しているか * 命名が適切かどうか * (API の時) テストが十分で不足していないこと * その他細かいもの * エラーハンドリングのミスなど ### まっさん 基本的には差分だけを見て指摘可能な事項のみがいいと思ってます（プルしてきての動作確認、修正漏れの検索、はコスト高いので対象外、ただし、他の部分との整合性の不備に関しては、気づいたら指摘するほうがいいかも） - セキュリティ的な懸念を表明する - テストケースの不備を指摘する - 別の（よりよい）やり方を提案する（すでにある関数での代用、より効率的なアルゴリズム、より意図の伝わりやすい名前、など） - 意図の不明瞭な部分について質問し、意図を理解した上でなおわかりにくいようであればコードを変えるかコメントを書くように促す そういえば、前に ab チーム向けにはガイドライン書いたんでした、ご参考まで。 [知/agent bank/開発/コードレビューガイドライン #guideline - scouter.esa.io](https://scouter.esa.io/posts/2987) あと、個人的にレビューコメントにはプレフィックスをつけてまして、直感的にわかるとは思いますが、いちおう明文化しておりますので、こちらもあわせて。 [GitHubでのコードレビューにてコメントにラベルをつける - Qiita](https://qiita.com/nunulk/items/ac35aec43194e72dc142)