👁️‍🗨️公開/🧑‍🎓学生向け/🏡TOP/学習を進めるにあたって/レビューガイドライン

レビューガイドライン

⚠️
課題の解答のコピーと参考の線引きについて
  • あくまで参考程度にするのであればOK
    • コピペすることは、決してダメなわけではない
  • 適当に数値を書き換えて動いた程度の理解力は思考停止状態なのでNG
あくまで活用程度に。
  • 何を書いたのか理解できていないとバグが多くなる
  • バグにすら気づかない理解できていないとバグの修正もできない
  • 自分だけでなく他のメンバーにも迷惑をかけるケースがあるので注意しましょう

レビューする方へ
  • コピペが見受けられた場合は必ず理解しているか質問してみてください
  • それでも改善されない場合はコース運営メンバーや社員へ共有をお願いします
 

はじめに

このページを見に来たあなた。この時点で素晴らしいです。GitHubのPullRequestを提出するときって自分が書いたコードを否定されるんじゃないかって思うことありますよね。
もしくは、課題が終わって逆にレビューをしなくならなくなってしまったあなた。今までは自分のコードだけ気にしてれば良かったですが、今度は人のコードを見てアドバイスや指摘をしなければなりません。レビューなんてコードが書ける人、上達した人がすれば良いと思うかもしれません。
コードが書けるようになるまで？いつでしょう？そんなの気長に待ってくれません。やらないと始まりません。
 
心配しないでください。このガイドラインを読めばきっと円滑にPullRequest(以下PR)を進めることができます。
YouTubeでも解説しています

このページの対象者

  • これからPullRequestを提出してレビューをしてもらう人
  • 始めてレビューする側になったけど、どうやってレビューするのかわからない人
レビューガイドラインではどこを見れば良いか、どんなマインドが必要かをまとめています
 
忙しい人は太字だけ読めばなんとなく理解できます
 

レビューとは

コード・ドキュメントの作成者以外の誰かが検査するプロセスです
  • 書いたコード、ドキュメントに問題がないか
  • 書いたコード、ドキュメントが全体の設計に合っているか
レビューはコードとプロダクトの品質を維持するために行われます。
 

レビューの目的

レビューの大前提はチームでの開発

LINEやメルカリを例に取ると、新機能の追加/不具合修正/UIの調整など頻繁にアップデートがされています。 規模が大きければ大きいほど(大規模サービスではコード行数が数百万行になったりします)1人で開発をしているわけではなく、社内でチームが組まれており、複数人で開発を行っています。
 

最大の目的は継続性の担保と問題の対処

無秩序で各々好きな様に開発を行うとどのようなことが起きるでしょう？
  • チーム全体で想定していた動きにならない
  • 自分が実装した箇所に関係ないところでエラーが発生
  • 条件分岐が複雑で読むだけで1日かかる、、 など
アサインされたばかりの新人でも手軽にコードを読める・書ける状態に保つことで開発がスムーズになります。
 

心構え

これから作るサービスの”１番のファン”である意識を持つ

クライアントによっては、新しい機能の追加や、機能の変更などの依頼がたびたび発生します。その時に、フレキシブルなコード実装でない場合、大きな実装修正を余儀なくされます。
常にサービスどのような使われ方をされるか想像し、実装する前段階でどのようなフローでサービスが使われるのが理想か、また、どのような実装方法であれば、変更などに柔軟に対応できるかを想定しながら実装するようにしましょう。
あなたは、作るサービスの”１番のファン” です。ファンであれば、例え技術が間に合っていなくても、ユーザー目線で真に必要であるべきことが見えてきます。感じたことや思ったことは積極的にプロジェクト内で発言しましょう。また、考えられるユーザーの動きを想像し、エラーやバグの危険性や、仕様の抜けもれがないか確認しましょう。
サービスに欠陥が存在していることを指摘し、皆がその問題を把握・対応の検討がされている状態は理想の形です。気付いてもひた隠しにする行為は、後に自分に手間として返ってくるので、思うことは積極的に発信しましょう。

レビューは対話をして認識を合わせる

誰もがレビュワーであること
  • タイプミス発見などはスキルに関係なくチームに貢献できます、勇気を持ってレビューしよう
改善としての`意見`を伝えること
  • 相手を否定しない、レビューに階級はないので忖度せずに伝える
わからなければ聞いてみること
  • チームでナレッジを共有できる
  • よくわからないと感じた部分は大体コードも怪しい
  • 設計時の漏れが発見できる
指摘だけでなく、良いコードだと思ったらそれも伝えよう
  • 自分が実装するときの参考になるし、素晴らしいコードを書いた人に感謝

つまり、レビューをOKするということは、提出物に対して同意すること

レビュー者はあくまでもチームの代表としてチェックするのであり、その人だけが責任を負うものではありません。
 

チェックするべき箇所(レビュー・レビュワー共通）

Googleが提唱しているチェック箇所

  1. 設計：コードは適切に設計されており、システムに適していますか？
  1. 機能性：コードは作成者が意図したとおりに動作しますか？コードの動作はユーザーにとって適切ですか？
  1. 複雑さ：コードをもっと単純にすることはできますか？別の開発者は、将来このコードに出くわしたときに、このコードを簡単に理解して使用できるでしょうか？
  1. テスト：コードには、正しく設計された自動テストがありますか？
  1. 命名：開発者は変数、クラス、メソッドなどに明確な名前を選択しましたか？
  1. コメント：コメントは明確で有用ですか？
  1. スタイル：コードはスタイルガイドに従って いますか？
  1. ドキュメント：開発者は関連するドキュメントも更新しましたか？
 

ポイントを抑えてチェックする

  • 同じコードが繰り返し使われていないか
  • 変数/関数名が適切であるか
  • 条件が複雑で無いか
  • 境界条件(0,null,empty)でうまく動くか
  • 読みやすいか
 

同じコードが繰り返し使われていないか

同じ処理が複数箇所で使われているのであれば共通化を検討する
DRY(Don't repeat yourself)の原則 
情報の重複は変更の困難さを増大し透明性を減少させ、不一致を生じる
 
ただし、過度な共通化は影響範囲が大きくなるので注意する
 

変数/関数名が適切であるか

英語間違ってない？主語、動詞、名詞の区別がついている？
Ex: CreateUser(ユーザーを作成する), UserCreate(ユーザーが作成する), UserCreation(ユーザーの作成)
書き方を理解する
  • キャメルケース、スネークケースなど
 
クラス名はアッパーキャメルケース(大文字スタート) 
  • ex) ArticleViewController, FeedListActivity
どんな変数か、どんな役割のクラスか、何をする関数かを具体的にする
  • image, text, get, だけではわからないので明確にする
  • 名前が短くてわかりにくいより長い方が良い
ネーミングに迷ったら使ってみる: https://codic.jp/
 

条件が複雑で無いか

まずはこのコードを軽く見てみてください。
読みづらいですね。。
 

ネストが多くなるに比例して複雑さが増す

実装者はまだしもレビューする側としてはパッと見で何をしているのかわかりづらくなる。
ex) Swift: switch, guardなど Kotlin: when, if letなど
 
考慮漏れなどの不具合の原因になるので読んだときに「わかりづらい」と感じるようであれば書き直しを検討してみる。
 

境界条件(0,null,empty)でうまく動くか

Rangeを意識しているか
  • foo > 0, foo < 0, foo >= 0, foo <= 0
Listの中身はnullだけでなくEmptyの状態も考慮する
  • nullチェックはもちろん isEmpty, isNullOrEmpty等でチェック
静的型付け出ない言語(Python, JavaScriptなど)を使う場合は、型を意識しているか
  • 考慮漏れがあると実行時に落ちます
 

読みやすいか(大事）

改善の余地がある例）
インデントを揃えているか空白を入れているか改行を入れているか
代替この辺が意識され、前述した命名やネストについて意識できていれば読みやすいコードになるlinterやスタイルガイドを導入しよう
 

レビューを依頼する側

Commitメッセージはわかりやすく

  • 実装した内容
  • 追加したのか、削除したのか、新規作成したのか、など
  • 日本語で良いので第三者が見ても理解できる内容にする
  • 無理に英語でメッセージを残す必要はない
 

レビュー依頼する側のガイドライン

  • PullRequest提出時にはレビューを依頼する人にメンションでURLを付与してメッセージを送る
  • セルフチェックを行う(チェックが完了するまではドラフトにする)
    • ビルドしてちゃんと動くか
    • レイアウトに崩れないか
    • テストが通っているか(Linterなど)
    • 影響範囲を考慮して実装できているか(定義元、呼び出し先の確認)
  • 環境変数などそのPR内で追加したものがあれば明記する
  • 実装時に調べたことや記事・サイトなどのリンクを参考として貼る
 

レビューをもらった箇所でやりとりを行うこと

 

まとめ

  • レビュー自体は初心者からでもできるし、ベテランだけが行うものではない
  • 質問と対話で進めていく
  • 良いところは参考にして取り入れていこう
  • コードレビューの目的を忘れて好き嫌いでコメントしない
  • レビュー自体も数をこなすことで慣れていくものなので最初から全部指摘しようなんて思わなくて良い