情報システム特別講座 :

アプリケーション開発で質の高いコードレビューを実現するためのポイントとは ? ~ 前編~

2020-09-01
How to be a Developer

森崎 修司
名古屋大学 大学院 情報学研究科 准教授

コードレビューはソースコードを目で追って問題がないか確かめる活動です。自分の書いたコードを他の開発者に確認してもらったり他の開発者が書いたコードを自分が確認します。そのほかにも、他のメンバーが書いたコードを理解したり、代替案を考えたりすることが目的である場合もあります。本記事では 9 月と 10 月の 2 回にわたって、問題を見つけて指摘(修正)することをコードレビューの目的として、どの箇所を見てどういうふうに確認すべきかを説明していきます。

*本記事では、レビューで指摘するものを問題と呼びます。代表的な問題はバグや欠陥ですが、指摘する時点ではバグや欠陥とは言えないものの将来引き起こしやすくなっているものを総称して問題としています。


コードの読み進めかた

問題がないかどうかチェックするために、確かめる箇所と確かめかたをコードの「読み進めかた」と呼びます。仕様との不整合をコードレビューで確認することは、多くのコードレビューにあてはまると思いますので、これを具体例として見てみましょう。 

確かめる箇所

仕様に対応するコード 

確かめかた

仕様とコードの間に不整合がないかチェックする。 

あっさりとしていますが、この「仕様」に該当する部分を仕様から取り出し、対応するコードを確認します。

もう少し具体的に、独自にオンラインショッピングサイトを開発している場合を考えてみましょう。

仕様に「会員ステータス、買い物の総額、キャンペーンといった条件で送料を無料にする」といった部分があるとします。これらを実現しているコードを確認して、条件を満たしていれば送料が無料になるような実装になっているかどうかを確かめるといったように確認していきます。このとき、上の「確かめる箇所」と「確かめかた」は以下のように具体的に書けます。

確かめる箇所

送料無料の条件を設定する箇所と送料を計算する箇所

確かめかた

送料無料の条件が仕様と同じになっているか確かめ、無料の条件が成り立っていれば送料が無料になっているかチェックする。 

確かめる箇所としてプロダクトコードを前提としましたが、テストコードがあれば、テストコードも確認する箇所に含まれます。

プロダクトコードであれば、その箇所が期待通り実現できているかどうか確かめます。テストコードでは、そのテストでバグがみつかるかどうかを確かめます。確かめかたは、送料無料の条件が正しく反映されるかです。同時に条件がそろっていなければ送料が加算されることも確かめます。

仕様として明示はされていないけれど、実現する必要のあるものを「暗黙の仕様」と呼びます。これも同じように確認しなければなりません。オンラインショッピングサイトの商品検索機能として

「検索キーワードを含む商品の一覧を表示する。1 画面に表示する結果は 30 件とする」

といった仕様が明示されているとします。このときの暗黙の仕様として

「検索結果が表示されるまでの時間は長くても数秒程度にする」

があります。表示に時間がかかるとユーザーがその機能を使わなくなるからです。この暗黙の仕様をあるべき姿と考え、確かめる箇所と確かめる方法を考えてみましょう。

確かめる箇所

検索機能を実現する部分

確かめかた

検索対象によっては検索結果を表示するまでに長い時間がかからないかチェックする

確かめる箇所は、検索対象となる商品情報のデータ構造、検索部分、検索結果を表示する部分です。データベースの検索 API を使う場合や SQL で問合せする場合には、その部分が確かめる箇所です。

確かめかたは、扱う商品種類数や同時に検索される回数が増えても短時間で検索結果が表示できるかどうかです。たとえば、データベースに保存した商品数が増えるにつれ商品検索にかかる時間が長くなり、100 種類で 0.5 秒、10,000 種類を超えると 10 秒かかるとします。商品種類数が少ないうちは問題とはいえませんが、商品種類数が多くなると問題です。

将来、10,000 種類を超える商品を扱うことがわかっている場合には、商品種類数が多くなっても検索時間が短くて済むような API を使ったり、アルゴリズムを選ぶ必要があります。


読み進めかたのバリエーション

コードレビューの経験がある方であれば、なんとなく自分のスタイルをお持ちではないでしょうか。それらのスタイルの多くは、読み進めかたとして分類できるはずです。

ご自身の経験がまだ十分でなかった場合、自分のコードや成果物をほかのメンバーにレビューしてもらうと、「そういう考え方があるのか」「そういうふうにはチェックしていなかった」と思ったことがあると思います。「指摘されれば理解できたけど自分で書いているときには気づかなかった」ことを身につけていったのではないでしょうか。

img_code-review-01

慣れてくると対象コードのレベルも推測できるようになり、「これくらい書けているコードなら、仕様との不整合はほとんどないだろうから、違う読み進めかたをしよう」、「この仕様の例外的な部分だけ、不整合がないか確認しよう」といった具合に、読み進めかたを選ぶはずです。それに加えて、「この部分がおかしいということは、こっちもおかしいかもしれない」といった、同時に起こりやすい問題も推測すると思います。

「この商品を買った人はこの商品も買っています」のバグ版で、「このバグを作り込んだ人はこのバグも作り込んでいます」を想像しながら、読み進めることがあると思います。

地道ではありますが、こういった読み進めかたを増やしていくことで、「問題がないかをこのくらい確かめておけば大丈夫」という実感が持てるようになると思います。そうした実感がなければ、「だいたい 30 分くらいみたからいいか」というような所要時間で質を判断するようなレビューになってしまいます。漠然とながめていていも、読み進めかたを考えてじっくり見ても、時間は過ぎていきます。自分なりの読み進めかたが身についてくると、問題がないかどうかを素早く判断できたり必要のない箇所を読み飛ばしたりして、問題を早く見つけることができるようになるはずです。

読み進めかたは、仕様との不整合だけではなく、様々なものがあります。コードの読みやすさはその代表例です。


将来の変更や拡張がしにくい箇所がないか確かめる

次に、変数名の命名の方針が揃っていなくてほかの開発者の誤解を招きやすかったり、データ定義、クラス定義、クラス階層の定義が不適切で、将来の拡張の際に作り直しが必要になったり、拡張しにくかったりするといった問題を見つける読み進めかたを見てみましょう。

たとえば、変数のスコープや変数名が適切でないと将来の拡張の際に困ることがあります。まぎらわしい変数名や適切でないスコープを確認する読み進めかたは、次のようにまとめられます。

img_code-review-02

確かめる箇所

変数を宣言、初期化する箇所

確かめかた

変数を宣言、初期化しているスコープよりもせまいスコープで変数を使っていないかチェックし、変数名の命名の方針が揃っているかどうかをチェックする

言語固有の勘違いしやすい書き方も同じように見つけることができます。次を見てみましょう。 


勘違いされやすいコード

以下は、C、 C++、Java で使われる switch case 構文です。switch 文の後の変数 value の値によって条件分岐するための構文です。

#### switch case 構文
int response (int value) {
    value += check_availability();

    switch (value) {
        case 0:
            return_value = 1;
            break; 
# break があるので次の case 1: 以降の文は実行されない。
        case 1:
            return_value = 0;
            # break がないと default の以降の文も実行される。
        default:
            return_value = -1;
    }
    return return_value;
}

switch (変数名) と書いて、case 値: と書くと、変数名の値ごとに実行する文を書けます。case のどの値にもあてはまらなかったときに default: の後の文が実行されます。if 文を羅列するよりも見通しがよくなるので、1 つの変数の値によって様々に分岐するときに使われます。

しかし、case の後に break をいれておかないと、次の case 以降の文も実行されます。

この構文の例では、変数 value の値が 1 (case: 1) にあてはまる場合でも、default 以降の文が実行されるため、switch 文の実行後の return_value は常に -1 になります。変数 value の値が 0 のとき (case: 0) のときには、break; があるので return_value は 1 となります。

このような問題を見つけるための読み進めかたは、次のようにまとめられます。

確かめる箇所

switch, case 構文 

確かめかた

case の後に break がないと意図しない実行結果にならないかチェックする

ここまでで、仕様との整合性、読みやすさを例として読み進めかたを説明してきました。要素技術に関しても同じように読み進めることができますが、要素技術に関しての読み進め方については後編でご紹介することにしましょう。

後編では読み進めかたの効率化の方法として、読み進めかたの整理と自動化の利用を紹介します。どうぞお楽しみに。

プロフィール

photo_nagoya-univ_morisaki

森崎 修司
名古屋大学 大学院情報学研究科 准教授

ソフトウェアレビュー、ソフトウェア計測を専門とする。コードレビュー (ソフトウェアレビュー)、ソフトウェア計測、実証的ソフトウェア工学に関する執筆活動多数。主な著作に『なぜ重大な問題を見逃すのか ? 間違いだらけの設計レビュー』(刊:日経BP) がある。

AWS のベストプラクティスを毎月無料でお試しいただけます

さらに最新記事・デベロッパー向けイベントを検索

下記の項目で絞り込む
絞り込みを解除 ≫
フィルタ
フィルタ
1

AWS を無料でお試しいただけます

AWS 無料利用枠の詳細はこちら ≫
5 ステップでアカウント作成できます
無料サインアップ ≫
ご不明な点がおありですか?
日本担当チームへ相談する