コードレビュー観点を整理してみた

初めに

最近はReact/TypeScriptをベースにフロントエンドの経験を積むことが多く、その中でコードレビューをする機会も増えてきました。その中で感覚的にレビューしているところあり、どのような観点でレビューすべきかを整理したいと思い、この記事を書きました。レビュワーの参考になると同時に、コードを書く人が事前にNGケースを回避してくれるようになるといいのではないかと思います。

コードレビューの観点

前提として、コードレビューは以下の観点に分けられると思います。

業務観点
非業務観点

業務観点とは、設計書やタスクチケットの内容に沿って実装されているか、という観点です。満たしていないと仕事をできたことにはならないと思います。非業務観点とは、上記以外の観点を指します。詳細は後述しますが、例えばより無駄なくスマートにコードをかけるかといったよりテクニカルな観点を指します。満たしている必要は必ずしもないが、満たしていないコードの品質は低く望ましくないと考えます。例えば、保守性の低いコードでは改修時に苦労します。

つまり、こういうことです。

観点名	概要	必須
業務観点	設計書通りに動くか？	○
非業務観点	品質の良いコードか？	△

業務観点はどのような時でも求められていますが、非業務観点は、例えばプロジェクトが炎上しているような場合など、プロジェクトの状況によっては省くことがあるということです。

以下、それぞれの観点について具体的にどのような点を見ていくべきかを記載していきます。

業務観点

業務観点のレビューでは以下の3つを見ています。

設計書通り動くか
設計書の考慮漏れはないか
errorやwarningが出ていないか

1. 設計書通り動くか

設計書に記載されてる通り期待された動きをしているかを確認します。「処理Aののち、処理Aの結果を踏まえてAPI①を呼び出し、レスポンスを踏まえて処理Bを行う」といった設計がされているとき、その通り動くかをブラウザで動かしてみてチェックします。なぜ、実際に動かしてみるのかというと、静的にコードを見るだけでは見落とすケースがあるからです。

例えば、動かしていると「処理Aの結果を踏まえて」の部分が上手く実装できていないことがあります。

// NG
// 処理A
const handleA = () => {
  // ここでAPI呼び出しで利用する値を更新する
  setState(...);
  // APIを呼び出す
  fetchAPI(); // 更新をまたずにAPIを呼んでしまっている
}

上記のようなコードは静的にコードを見ているだけではlintなどもerrorを出しませんし見落としてしまう可能性があるので、設計書通りに一通り動かしてみることが重要です。

また、以下のようなコードは2回APIを呼び出してしまいます。

// 処理Aで更新するstateを監視してAPIを呼び出す
useEffect(()=> {
  // API呼び出し
  fetchAPI();
},[state])

stateを初期化した際と、stateが更新された際にこのuseEffectのcallBackが動くので2回APIを呼んでしまいます。更新時のみfetchAPIするように修正するなどの対応が必要です。

これもまたlintなどを乗り越えてしまうので動かしてみることが大切です。

動かしているときに見るもの

動かしている際に見ているものとしては主に以下の内容が挙げられます。

レイアウト
ブラウザのNetworkタブ

レイアウトはそのままで、適切に画面描画ができているかを見ています。ブラウザのNetworkタブはリクエスト・レスポンスが適切かを見ています。ここを見ながら操作していると先のように更新を待たずリクエストしたり、不要に2回リクエストしているコードに出くわします。

Networkタブではこんな感じで通信しているものが記録されます。

また、クリックするとパラメータやレスポンスなど詳細を見ることもできます。

これらを見つつ、期待したパラメータとなっているかをチェックしていきます。

設計書通りかを確認する際に留意すべきこと

プロジェクトによっては例えば区分値の定義やエラーハンドリングなどが共通の仕様として別の設計書に定義されていることがあります。個別の画面についての設計書だけでなく、共通の設計書とも相違なく実装できているかを確認することがレビューでは重要になります。

2.設計書の考慮漏れはないか

ウォーターフォールの場合、基本的に手戻りは避けたいのですが、残念ながら完全な形の設計書はそうそう出会えません。多くは何らかの記載不備などを抱えています。また、ReactはSPA(Single Page Application)ですが、設計者がSPAの特性を理解せず設計書を書いているケースも存在します。また、要件追加が上手く考慮されていないといったケースもあるでしょう。このような場合、考慮漏れなどが存在する場合があります。例えば、画面Aからの遷移のみを想定して設計書が書かれているが、画面Bからも遷移することが考慮されていない、といったケースがありえます。こうしたケースでは該当箇所の編集内容を静的にコードを見ても見落としてしまう可能性が高いので実際に動かしてみることが大事になります。設計書上は想定していないが実は画面Bに遷移するボタンがあるかもしれません。

ユーザの気持ちになり、色々なボタンを押したりブラウザバックしたりリロードしたり、色々なことをやってみます。そして大抵どれかの操作で壊れます。この際、ソースコードを書いた人にフィードバックすることは当然ですが、設計者にもフィードバックしておくことが重要です。

3.errorやwarningが出ていないか

ブラウザ上でガシガシ動かしているとconsole上にerrorやwarningが出ることがあります。ぱっと見はちゃんと動いているように見えても問題のあるコードなので修正するようにコメントを入れましょう。

ブラウザのconsoleを見ると様々logが出てきます

consoleを用いたdebugは色々な機能がありますので参考記事を提示しておきます。参考: https://qiita.com/ashketcham/items/06e50b3f7f6238d9b51b

非業務観点

非業務観点では以下の観点からレビューを行っています。

タスクチケットの範囲か
コーディングの基本に忠実か

1.タスクチケットの範囲か

タスクチケットの範囲かを考えるのも重要かなと思います。よくできるプログラマーはついつい手癖でタスクチケット範囲でない内容も修正してしまうことがあるのですが、同件が別画面に潜んでいるかもしれません。直した気になって他の画面で同じバグを引くのはもったいないです。かといって別画面も同時に直してしまうと修正diffがあまりに大きくなってしまいます。なので、別のタスクチケットにして調査の上対応するのが望ましいでしょう。一画面のプログラマーよりも多くの画面を見るレビュワーの方が視野を広く持てるはずなので、レビュワーの方がこうした判断はより適切にできるのではないかと思います。そのため、上記の観点で修正をレビューします。関係のない修正は基本的に排除していきます(あまりに小さなフォーマットなどであれば同タスクチケットでの修正も許容しうるとは思います)。

2.コーディングの基本に忠実か

何がコーディングの基本かというのは難しいところで、流派も色々あると思います。しかしながら、以下の観点については重要だと多くの場合合意が取れるのではないかと思います。困難は分割し、より簡素に書くことが大方針です。

命名規則に忠実であるか
変数・関数のスコープは最小限にする
長すぎるロジックは分割する
共通のロジックは共通のロジックとして切り出す
言語を理解して実装する
ディレクトリ構成に従ったところにロジックを書く
コメントを入れる

命名規則に忠実であるか

変数・関数名を適当につけると、コードの可読性・保守性が下がります。例えば、関数などでは処理内容に対応していない命名がなされることがあります。

// NG
const makeValidItem = (itemList:Item[]) => itemList.find(item=>...);
// 検索する処理を実装しているのにmakeを使っている

// OK
const findValidItem = (itemList:Item[]) => itemList.find(item=>...);
// 実装内容に対応した名前が付けられている

こうしたコードは一見して何をやろうとしているかわからないので、レビュワーが指摘すべき内容だと思います。

その他、JavaScript向けにかなり洗練されたnaming ruleがまとまっておりますので、参考に提示しておきます。

参考：

https://github.com/kettanaito/naming-cheatsheet

現場ではこちらを参考にいくつか加筆・修正したnaming ruleを用意しています。

変数・関数のスコープは最小限にする

例えば、以下のようなコードの場合、スコープが必要より広くとられています。

// NG
const weight = 1;

if(...) {
  const result = sum * weight;
  //  以下略
}

// 以降、weightは使わない

このような場合はif文内で宣言したほうがよいでしょう。weightが想定しない使われ方をしてしまうかもしれません。

長すぎるロジックは分割する

業務用のコードの場合、処理が幾重にも重なりコードの行数が長くなることがあります。適切に分割されるべきでしょう。例えば1000行近くのComponentなどを見せられてもレビュワーは正しく判断できません。というか見るのもいやです。一見して何をやるComponentなのかわからないので保守性も低いです。例えば、商品一覧を表示する画面の際、すべての内容を盛り込んだ一つのComponentを作るのではなく、1商品ごとのComponentを作り、それを利用する商品一覧Componentを作るという構成にします。こうすることで、商品の内容による表示制御(例えば在庫切れの時は注文ボタンが押せないようにするとか)は商品Componentの責務となり、商品一覧ComponentはAPIで取得した商品リストのデータを商品Componentに渡すだけでよくなります。

// NG
const productList:Product[] = findAllProductData();

return productList.map((product)=> {
　// ここから内容ごとの処理が始まる…
  return <div>
           <div>商品名<div/>
           <div>{product.name}<div/>
           {/* 在庫切れの時は注文ボタンを非活性にする */}
           ...
         <div/>
})

// OK
const productList:Product[] = findAllProductData();

return productList.map((product)=>{
  // 具体的な商品ごとの制御はProductでやるのですっきり！
  return <Product product={product} />
})

ReactにおいてどのようにComponentの分割をすべきかについては様々な流派があり、語りつくせないので割愛させていただきます。

共通のロジックは共通のロジックとして切り出す

例えば、文字列フォーマットなど、画面をまたいで共通で必要となるロジックが存在します。こうしたものはまとめてUtilに定義することが望ましいです。一画面の担当者よりも多くの画面をレビューするであろうレビュワーの方がこうした点により気づきやすいと思います。また、同じ画面でも、共通のロジックが潜むことはよくあります。これらを意識してコードを読むとコードをより簡素にする提案ができると思います。また、すでにある共通のロジックの存在をプログラマーは気づいていないかもしれません。こうした知見を与えるのがレビュワーの役目だと考えています。

言語を理解して実装する

言語にはそれぞれ仕様や組み込みの機能がありそれを理解して実装することでバグを防いだり、可読性を上げることができると思います。例えば、JavaScriptは配列についてfind、someなど複数の機能が実装されており、それを使うだけで一見して何をしたいのか理解できるようになっています。

// NG
// 有効なアイテムを探す
let validItem: Item;
// リストからforEachで探す
itemList.forEach((item)=>{
  if (item.isValid) {
    validItem = item;
  }
});

// OK
// 有効なアイテムを探す
const validItem = itemList.find((item)=>item.isValid);

また、分割代入・スプレッド構文という仕組みにより配列やオブジェクトを扱うときに見通しが良くなります。

const cola = {name: "cola", price: 160};
const { name, price } = cola; // name = "cola" price = 160

const cola = { name: "cola" , price: 160};
// 基本的なプロパティを踏襲しつつ必要なものだけ変える
const soda = { ...cola, name:"soda"};

また、三項演算子、Nullish Coalescingや短絡評価もコードをより宣言的で見通しよく書く上では必要です。

// NG
let b:number; // b が他の箇所で編集可能になってしまうし、bにどのような値が入るかが手続き的に書かれ複雑になると可読性が下がる
if(a === null) {
  b = 100;
} else {
  b = 200;
}

// good
const b = a === null ? 100 : 200; // bは定数として宣言できる

// NG
const a = null;
let b:number;
if(a === null) {
  b = 100;
}

// OK
const a = null;
const b = a ?? 100; // b = 100

言語によってさまざまな仕様や機能があると思いますので、それらをよく活用し、より簡潔で可読性の高いコードを目指すことが目的になります。レビュワーはコードを書く人より経験値が高いはずなので、こうしたより技術的に高度な観点からの指摘をすることが求められると思います。

ディレクトリ構成に従ったところにロジックを書く

プロジェクトによりますが、各ディレクトリの役割が決まっているのであればその役割に沿った箇所にロジックは記載されるべきでしょう。例えば、repository/というディレクトリがあるのにDBを操作するロジックがContainer内に書かれることがあります。後から見る人はどのディレクトリ構成などを見つつ、どこにどんなロジックが書いてあるか想像するはずなので、あるべき場所にあるべきロジックがあることは大切です。

コメントを入れる

いかに困難を分割しても、分割しきれない複雑さを持ってしまうことはあり得ます。そうしたケースでは適切にコメントを書くようにレビューするのがいいと思います。コメントを入れておかないと、後で見たときに何をやっているかわからず読み解きに時間がかかります。また、現時点では対応できず別のタスクチケットにしている場合などFIXMEコメントを入れておくとあとから修正が簡単になります。

// OK
// FIXME: #34で対応予定

const filteredItemList = itemList.filter((item)=>{
  // 処理内容のコメントをつける
  if(...){
    // 処理 
  }
});

よりReactにフォーカスした話をすると、useEffectなどを利用する場合、useEffectは宣言的ではなくそれがどのような意図で利用されているかがわかりにくくなりがちであるため、適切にコメントを入れることが重要になると思います。

終わりに

以上が私がレビュー時に考慮している観点になります。Reactによりフォーカスした観点などは記載していないので実際にはより多くのことを考えながらコードを読んでいます。レビューって疲れるなと思っていたのですが、これはそうならざるを得ないなと改めて感じました。よりReactにフォーカスしたケースについては気が向いた時にまた整理してみようと思います。ではまた。

記事一覧に戻る