コードレビューのやり方、基礎の基礎 - コード改善に重要なレビューの基本的な考え方を学ぼう

コードレビューとは?レビューで問題を見つけて指摘するには?レビューをされる側の心構えとは?ソフトウェアレビューを研究する名古屋大学の准教授 森崎修司さんが、コードレビューの考え方を解説します。

コードレビューのやり方、基礎の基礎 - コード改善に重要なレビューの基本的な考え方を学ぼう

はじめまして森崎です。大学でソフトウェアレビューの研究をしています。さまざまな組織との共同研究、調査、議論を通じて、レビューの原理・原則や体系的な考え方・知識を明らかにしようとしています。大学で研究に従事する前にソフトウェアエンジニアとしてインターネットサービスの開発をしていたため、研究として価値があり、実務としても役に立つ研究を目指しています。

レビューは、とにかく多くの経験をつまなければ上達しないという先入観を持たれがちです。その先入観をなくして、レビューの上達やソフトウェア品質の向上につながるしくみや活動を明らかにし、現場のエンジニアのみなさんに知ってもらう活動をしています。

本稿では、これから初めてコードレビューをする / される方を対象に、レビュー実施前に知っておいていただきたい基本をお伝えします。

コードレビューとは?

コードレビューはソフトウェア開発活動の1つで、いくつか目的があります。もっとも一般的な目的は、コードを目で追って問題1がないか確かめることです。自分の書いたコードをほかの開発者が確認したり、ほかの開発者が書いたコードを自分で確認したりします。

そのほかの目的として、他のメンバーのコードを理解して、自分のコードとの間で齟齬がないかを確認したり、問題がある部分の代替案を考えたりする場合もあります。1回のレビューでこれらすべてを目的にすることはあまりなく、1回目のレビューでは欠陥がないかを確かめ、2回目のレビューでは代替案がないかを確かめる、といった具合に使い分けるのが一般的です。

コードの「読み進め方」

どのような目的のレビューでも、確かめる箇所と確かめ方を決めてからレビューすると効率的ですし、レビューの質も上がります。このような、コードの「読み進め方」が定まっていないと、ただ漠然とコードを読み流してしまいがちです。

ただし、こうした読み進め方を手当たり次第に身につけて、とにかくたくさんの問題を指摘できるようになるだけでは「レビューのスキル向上」とは言えません。レビューのスキルは、大きく2つに分けることができます。

1つは問題を見つけるスキルです。読み進め方をたくさん知ることも含まれますが、それを適切に選ぶことやチームで合意することも含みます。もう1つは見つけた問題をうまく指摘するスキルです。見つけた問題を、どのように伝えるかで、問題が解決するスピードや精度は変わります。迅速に問題を解決するには、コードを書いた人をはじめ、チームに問題をうまく伝える力も求められます。

レビューは見よう見まねで続けられていることが多い

個々のコードレビューで、「読み進め方」が具体的に決められていることはあまり多くありません。過去にそうやっていた、ほかの開発者がやっているからといった理由で、なんとなく決まっている、という状況もあるでしょう。

レビューで指摘できることはかなり広範囲にわたるため、本来見つけ出したい問題以外の問題が指摘されることもあります。たとえば、再設計して実装をやり直すことが決まっているのにコードの拡張をしやすくするための指摘をしていたり、将来においても性能が求められないにもかかわらず、性能向上すべき点を指摘したりしていることもあります。

これからやろうとしているコードレビューの目的を理解し、「読み進め方」を選ぶことでレビューの質を高め、効率化にもつなげることができます。

適切な方法でレビューを進める

「レビュー指摘」という名目で、コードを書いた開発者を責めてしまうことは少なくありません。「必要ないかもしれないけれど、自分のコードレビューのときには言われたから」といった理由で、理不尽な指摘や要求をしているのを見たことはないでしょうか。また、自分のほうが技術的な知識が多いことを示すことが主な目的となって、必要のない指摘をしているというケースに遭遇した経験を持つ方もいるでしょう。

こういった、プロダクトの質を高めることにつながらない指摘を避けることは、レビューで求められる基本的なスキルです。なぜなら、レビューの効率化につながるからです。さきほど挙げた例のように、プロダクトの質とあまり関係のない問題を見つけようとしていれば、まずは目的を見直す必要があります。

レビューの目的を理解する

ただ漠然とコードをながめても、ある程度はレビューを進めることができます。その一方で、コードの分量が多くなったり、時間制約があったりすると、レビューが難しくなります。 そのような場合には、以下の例のように、問題が起きそうな部分を中心に確認します。

例1)別々の開発者が書いたメソッドや、関数間でやりとりされるデータに不整合はないか 例2)入力値やパラメータによっては実行時間が極端に長くならないか

おおまかに「ここでこの問題があったら、こういうバグにつながる」という問題がイメージでき、その問題を見つけるための「読み進め方」がイメージできていれば、レビューの目的が決まったと言えるでしょう。

目的を決めることで、"近々再実装することが決まっているのに、拡張性に関する問題をたくさん指摘される"とか、"性能やセキュリティ上の問題がないかを確かめることが重要なシステムにおいて、拡張性の問題をたくさん指摘される"といったレビューの行き違いが防げます。

複数人でレビューをするときは、ほかのレビューアと同じ目的を共有しているか確かめてください。目的が決まっていなければ、そのレビューに求められる目的を話し合い、事前に合意しましょう。

組織標準のチェックリストがある場合や目的が決まっている場合は、それに沿って進めます。レビュー会議で「そのタイプの問題は、今回は指摘する必要はないと思う」といった議論が始まると、レビューの時間がなくなってしまいます。また、参加者が「そういう問題は指摘しなくてもいいのでは」と感じてしまうレビューも同様です。こうしたことが起こらないように、レビューを始める前に目的を確認・合意しておくのが重要です。

問題を見つけよう

コードレビューの原則は、あるべき姿とコードを対応づけて問題を見つけることです。自分なりの「読み進め方」を身につけると、問題がないかどうかをすばやく判断できたり必要のない箇所を読み飛ばしたりできます。そうすると、問題を早く見つけることができます。

では、「読み進め方」の具体例を見ていきましょう。本記事では、3つのタイプを紹介します。

読み進め方その1 「バグがなさそうか」を確かめる

1つ目のタイプは、バグ(期待する動作をしない)があるコードです。もっともわかりやすく、あるべき姿とコードを対応づけてバグを見つけるための「読み進め方」です。オンラインショッピングサイトの開発であれば、以下のようなバグが考えられます。

  • 画面で指定した商品とは違う商品が注文される
  • 画面で指定した数量とは違う数量の商品が注文される
  • 登録されている商品名とは違う商品名が表示される

これらを実現するコードがどこにあるかを特定して、正しく実現できているかどうか確かめましょう。テストコードがあれば、そのテストコードでバグが見つかるかどうかも確認します。 このような問題を見つけるための確かめ方は、以下の3つをチェックすることです。

  • 指定した商品と同じかどうか
  • 数量が同じかどうか
  • 登録されている商品名と表示される商品名が同じかどうか

そのため、商品がどのように識別されるか(IDでの管理やそれが重複しないためのしくみ、検索時にどのデータ項目が検索対象になるかといった点)、数量はどこで指定され、処理されるかといった点を確認します。 バグの検出はテストでも行えますが、コード上でないと見つけにくいバグや大規模な修正が必要になりそうなバグは、コードレビューで確認するとよいでしょう。

レビューでは、バグのような「期待する動作との違い」だけではなく、ある条件下で問題になるようなコードや、将来の拡張において問題になりそうなコードといったタイプがないかを確認することがあります。それぞれ見ていきましょう。

読み進め方その2 「ある条件下で問題になる箇所」を確かめる

2つ目のタイプは、ある条件下で問題になるようなコードです。以下の図はC、C++、Javaをはじめとするさまざまなプログラミング言語で書ける多重ループの例です。数値v1、v2を与えて、clearという関数を呼びます。clear関数の実行内容やv1、v2の値によっては、実行時間が長くなる可能性のあるコードです。v1やv2が10以下であれば、clear関数の実行は100回を超えることはありません。しかし、v1とv2が両方とも1000を超えると、clear関数の実行は100万回を超えます。そのため、clear関数の実行時間と勘案して、100万回を超えて実行される場合でも問題がないか、もし問題がある場合にはこの書き方でないと実現できないかどうかを確認し、よりよい書き方を考える必要があります。

void test (int v1, int v2) {
    int i, j;
    for (i = 0; i < v1; i++) {
        for (j = 0; j < v2; j++) {
            clear(i, j);
        }
    }
}

ループの回数によって時間がかかる可能性があるコードを見つける読み進め方は、以下のようにまとめられます。

  • 確かめる箇所:多重ループ
  • 確かめ方:ループの繰り返し回数によっては極端に実行時間が長くならないかどうかチェックする

読み進め方その3 「コードの拡張性」を確かめる

3つ目のタイプは、将来コードを拡張する際に、コードを読む人を勘違いさせやすいコードです。コードを読む人にとって、紛らわしい書き方をしていないかどうかを確認するための読み進め方を紹介します。

次のコードを見てください。C、C++、Javaで書ける、紛らわしい条件分岐の書き方です。コードの3行目には2つのif文が書かれています。1つ目のif文の条件が満たされているときには、value++が実行されます。その次のif文は1つ目のif文の条件が満たされていないときでも実行されます。しかし、1つ目のif文の条件が満たされているときだけ実行されるようにも読めます。つまり、2個目のvalue++がv1とv2の両方がNULLのときにしか実行されないのか、v2がNULLであれば(v1がNULLでなくても)実行されるのかがわかりにくくなっています。

int test (int v1,int v2) {
    int value = 0;
    if (v1 == NULL) value++; if (v2 == NULL) value++;
    return value;
}

紛らわしい条件分岐を見つける読み進め方は、以下のようにまとめられます。

  • 確かめる箇所:if文の後の中カッコを省略しているところ
  • 確かめ方:条件を満たしたときに実行される文が、紛らわしく記載されていないかどうかチェックする

では、このコードをどのように修正すれば、勘違いが起こりにくいコードになるでしょうか。2つの改善案を紹介します。コードレビューのやり方によっては、こうした改善案を提案する場合もあります。

1つ目のif文の後のvalue++の後で改行(改善案1)するか、value++の前後に中カッコをいれて明示(改善案2)したほうがわかりやすくなります。

改善案1

int test (int v1, int v2) {
    int value = 0;
    if (v1 == NULL) value++;
    if (v2 == NULL) value++;
    return value;
}

中カッコを追加したコードが次のコードになります。

改善案2

int test (int v1, int v2) {
    int value = 0;
    if (v1 == NULL) {value++;} if (v2 == NULL) {value++;}
    return value;
}

地道ではありますが、こういった「読み進め方」を増やしていくことで、「問題がないかをこのくらい確かめておけば大丈夫」という実感が持てるようになります。そうした実感がなければ、「1時間見たからいいか」というような所要時間ありきのレビューになってしまいます。漠然とながめていていも、確認すべきところをじっくり見ても、時間はかかります。レビューの質を安定させるために、「読み進め方」を増やしていくのが重要なのです。

別のところにも流用できる読み進め方が増やせていても、最初は、どれも違う問題に見てしまい、同じような問題が見つからないのではないかと思いがちです。しかし、読み進め方のバリエーションを増やしていくと、あてはまる問題が出てきます。とくに似たようなプロダクトやドメインの開発をしていれば、読み進め方を流用できる機会が増えます。もちろん、レビューだけではなく、自分でコードを書く場合にもあてはまることが増えます。

見つけた問題を指摘しよう

エンジニアHubに会員登録すると
続きをお読みいただけます(無料)。
登録のメリット
  • すべての過去記事を読める
  • 過去のウェビナー動画を
    視聴できる
  • 企業やエージェントから
    スカウトが届く