Reviewboardによるプリコミット型レビューの運用を通して〜コードは見てもらうもんだ

こんにちは。むらしです。

現在コードレビューにReview Boardというツールを用いて運用をしています。Review Boardは米VMWare社がコードレビュー時に使用していたツールで、現在はオープンソース化しています。

Review Boardによる運用を通して思ったこと、感じたことをこの記事のテーマとします。記事の最後に、簡単ではありますがツールの使用方法なども記載します。

〇コードレビューとは？

そもそもコードレビューとは何なんでしょう。プログラマの格言にこんなのがあります。

「プログラムは思った通りには動かない。書いたとおりに動くのだ」

機械はバカ正直なため、書いた通りに動きます。「バグだ！」と思うのは人間だけなのかもしれません。

実装ミスを減らすためのツールはたくさん存在します。Lintツールやエディタの補完機能を使うことにより、文法的なミスやコーディング規約から外れたコードは早期に発見出来ます。こういう作業は機械がやるべきです。人間は間違えますし、それが自然です。しかし、コードレビューはやはり必要となります。例えば、それらのツールでは仕様漏れなどは検知出来ないでしょうし、変数名についてあれこれ思考錯誤する楽しみもないでしょう。
それに他人の意見を聞くということは、プログラミングに限らずとても大切なことだと個人的に思います。上達というのは以下のフローの連続であると思います。

自信満々に実際やってみる
やってみたことに対して分析して落ち込む
悪い部分を意識
最初に戻る

知識はやればやるほどついてくるので問題ないのですが、悪い癖を直すのは自分では気付きにくいものです。完成度を高めるというのは、ひたすら悪い箇所をなくしていく、という無限ループです。知識の習得と同時並行でこの作業を行わないと、恐らく変なプライドだけが高くなり悪循環です。プライドは大事ですが、変なプライドは上達を阻害するのではないでしょうか。世の中の職人と呼ばれる人達は、無限ループに重みを置いているのだと個人的に思っています。そういう人達は研ぎ澄まされていますし、技術者ならこちらを目指したいですよね。

〇プリコミット型レビュー、ポストコミット型レビューとは？

レビューするタイミングの違いとなります。ローカルリポジトリの内容をリモートリポジトリに反映させる前に行うレビューが「プリコミットレビュー」、反映後に行うのが「ポストコミットレビュー」となります。「プリコミットレビュー」の利点と欠点は下記の通りです(個人的意見)。
※「依頼者」は指摘される人、「依頼人」は指摘などを行う人とします。

[プリコミット型レビューの利点]

リモートリポジトリのコードにバグが入らない
リモートリポジトリのコミットログが汚れない

バグが入った場合、リモートリポジトリにバグ修正版が新しく反映されるはずです。正常に動作しないコードがリモートリポジトリに存在するのは気持ち良いことではありません。そしてバグが入ってしまった場合、リモートリポジトリに修正版を反映させる際にコメントを入れるはずです。その際のコメントに、「RevisionXXXのバグ修正」というログは残したくありません。後で見るとき、非常に手間になります。

[プリコミット型レビューの欠点]

依頼人が実際に動作を確認出来ない
差分ファイルを依頼者しか保持していない状態が長くなる

気にすることでもないかもしれませんが、個人的には気になります。

〇Review Boardとは？

Review Boardは「ポストコミット型レビュー」と「プリコミット型レビュー」両方に対応したツールとなります。冒頭でも触れましたが、米VMWare社でコードレビューを行う最に使用されていたそうです。様々なバージョン管理システム(CVS、SVN、ClearCase、Perforce、Git)や、バグトラッキングシステム(Redmineなど?)との連携が行えます。

私は「プリコミット型レビュー」によるReview BoardとSVNを連携させて運用していました。個人的な意見になりますが、このReview BoardというツールはSVNなどの単一リポジトリ型のバージョン管理システムと相性がよいのではないか、と思っています。一本しか道がない時、人はでこぼこの道より整備された道を好むのではないでしょうか。「プリコミット型レビュー」により、不要ログが少なくなります。Gitなどの分散管理型のバージョン管理システムは、機能単位などでリポジトリを分散させ、ある程度進んだら全員共有リポジトリ(master)に取り込んでいくため、そもそも全員共有リポジトリに不要ログが入ることは少ないと思います。

〇実際の運用に向けて

それぞれのレビュー型に分けて記載します。

[プリコミット型レビュー]

プリコミット型のフローを説明します。ここではプログラマらしくコード(JavaScript)でフローを説明します。関数「preCommitReview」がフローとなります。バグが入っていたら、ごめんなさい。他サイトでは図を用いて説明していますので、そちらも参考にしてみてください。

const submitter = {
    name: 'John Coltrane',
    modifyCode: function() {
        console.log('修正中...');
    }
};
const reviewer1 = {
    name: 'Charlie Parker',
    review: function() {
        /* return true or false */
    }
};
/**
 * @param {依頼者} submitter いざ、出陣
 * @param {依頼人達} reviewers 受けて立つ
 */
function preCommitReview(submitter, reviewers) {
    console.log(submitter.name, 'これでどうでしょう？');
    let shipIt = reviewers.length;
    for (let i = 0; i < reviewers.length; i++) {
        if (reviewers[i].review()) {
            console.log(reviewer.name, 'ship it!');
            shipIt++;
        } else {
            console.log(reviewer.name, '全然だめ。');
        }
    }
    if (shipIt !==  0) {
        console.log(submitter.name, '出直してきます。');
        submitter.modifyCode();
        preCommitReview(submitter, reviewers);
    } else {
        console.log(submitter.name, 'やった!リモートリポジトリに反映させますね！');
        return;
    }
}

preCommitReview(submitter, [
    reviewer1, reviewer2, ...
]);
commitToRemoteRepo();

あれ？これはおかしくないか。その通りです、おかしいですね。バグが潜んでいます。このフローであると、依頼者のレビューは、依頼人に承認されることはないでしょう！上記コードを見て、依頼人は下記のコメントを残しました。

(1) 11行目で変数「shipIt」をインクリメントしていますが、ことはデクリメントしないと無限ループになりませんか。
(2) 変数「shipIt」は何を表しているのか分かりづらいです。どういう意味があるのでしょう。
(3) 8行目でfor文を使用しています。配列全てを回したいのであれば、「for ... of」構文も使用可能です。
(4) 16行目で、「0であるか」を判定条件としています。「shiptIt」変数は依頼人からの許可数を表していると思いますが、「shipIt」をインクリメントして、判定条件を「依頼人の合計数であるか」としてはどうでしょう。その方が直感的です。
(5) この関数を非同期にしてはどうでしょう(レビューしている間、私はずっと待ちたくないよ！)。

上記4点はそれぞれ、コードレビューのメリットとなります。良いことづくめですね。
(1) 不具合、仕様漏れの発見
(2) 自分にしかわからないロジックや説明文、名前の発見
(3) 新しい知識の取得
(4)と(5) よりよいコードへの提案

そして依頼者は指摘された関数を修正して、再度レビュー依頼を出します。この時点ではリモートリポジトリに反映を行いません。

/**
 * @param {依頼者} submitter いざ、出陣
 * @param {依頼人} reviewers 受けて立つ
 * @return {Promise}
 */
function preCommitReviewAsync(submitter, reviewers) {
    function reviewRecursive(submitter, reviewers) {
        console.log(submitter.name, 'これでどうでしょう？');
        // Review Boardの用語となります。依頼人がレビューしてリモートリポジトリへの変更に許可を出すことを「Ship It!」と呼びます。
        // 依頼者が全員Ship It!すると、レビュー終了となります。
        let shipIt = 0;
        for (const reviewer of reviewers) {
            if (reviewer.review()) {
                console.log(reviewer.name, 'ship it!');
                shipIt++;
            } else {
                console.log(reviewer.name, '全然だめ。');
            }
        }
        if (shipIt ===  reviewers.length) {
            console.log(submitter.name, 'やった!リモートリポジトリに反映させますね！');
        } else {
            console.log(submitter.name, '出直してきます。');
            submitter.modifyCode();
            reviewRecursive(submitter, reviewers);
        }
    }
    return new Promise((resolve, reject) => {
        try {
            reviewRecursive(submitter, reviewers);
            resolve();
        } catch (e) {
            reject(e);
        }
    });
}

preCommitReviewAsync(submitter, [
    reviewer1, reviewer2, ...
])
    .then(() => {
        commitToRemoteRepo();
    })
    .catch((e) => {
        console.warn('レビュー終わらないから止めましょう。納期近いし。' +  e);
    });
submitter.anotherTask();
reviewer1.anotherTask();

修正版のコードに対して、恐らく指摘があるでしょう。1回のレビューのみ、とは限りません。参加者が納得するまで続きます。上記のコードには問題があります。anotherTaskがすぐに実行されない可能性があります。気になる人は考えてみてください。

最終的に全依頼人から許可をもらい、依頼者はリモートリポジトリへ差分を反映させます。Review Boardによるレビューは再帰的であり、時間を無駄にしないものです。

[ポストコミット型レビュー]

まずリモートリポジトリへ差分を反映させます。その後はプリコミット型レビューと同じフローとなります。

const postCommitReview = preCommitReview;
firstCommit();
postCommitReview(submitter, [
    reviewer1, reviewer2, reviewers3, reviewers4, reviewers5
])
    .then((res) => {
        commitToRemoteRepo();
    });

○運用開始のための環境構築

サーバーとクライアントの設定が必要となります。

[サーバー]

構築方法は割愛します。bitnamiで環境構築済みのVMイメージをダウンロードする方法が一番簡単です。
ブラウザの管理者メニューから対象のSVNのパスを指定しましょう。

[クライアント]

Review Boardはブラウザ上で差分を確認し、コメントするツールとなります。もちらんブラウザ上からレビューをリクエストすることも可能となります。しかし、間違いなくコマンドラインツールの「RBTool」を使った方が楽ですし、入力ミスなどもありません。インストールしたらバージョンを確認しましょう。コマンドは下記です。

rbt -v

私の環境では「RBTools 0.7.10」が出力されます。

(1) まずRBToolを使える環境を用意しましょう。依存ツールは下記2点となりますので、自前で用意します。

上記2点をコマンドプロンプトで実行出来るように、環境変数PATHに実行ファイルへのパスを設定してください。

(2) 次にRBToolの設定ファイルを作成します。プロジェクトのルートフォルダに「.reviewboardrc」を作成します。内容は下記となります。

REVIEWBOARD_URL = 'http://192.168.X.XXX' （REVIEW BOARDのIPアドレス）
REPOSITORY = 'TestSVN' （リポジトリ名）

試しにプロジェクトルートで下記コマンドを実行してみましょう。

rbt diff

エラーが発生しましたか？慌てずに、、、エラー出力内容は割りと親切です。

ERROR: The current directory does not contain a checkout from a supported source code repository.

svnが適切に設定されていないないようです。環境変数PATHへの設定は正しいですか？

CRITICAL: GNU diff is required in order to generate diffs. Make sure it is installed and in the path.

GNU diffが適切に設定されていないようです。環境変数PATHへの設定は正しいですか？

(3) RBToolの基本コマンド
以下のコマンドを覚えておけば十分です。

rbt diff

次回の新規レビューリクエスト時に投稿される差分が出力されます。ローカルリポジトリとリモートリポジトリの差分となります。

rbt diff リビジョン番号A：リビジョン番号B

リビジョン番号Aからリビジョン番号Bまでの差分が出力されます。

rbt post

初回レビューリクエストを作成します。ローカルリポジトリとリモートリポジトリとの差分となります。その際にリクエスト番号が振られます。この番号は再レビューリクエストを行う際に必要となります。

rbt post -r リクエスト番号

指摘点を修正して再リクエストレビューを行います。前リクエストと今回修正分の差分となります。

rbt post リビジョン番号A:リビジョン番号B

ポストコミット型レビューのコマンドとなります。レビュー新規でレビューリクエスを作成します。トリモートリポジトリのAとBの差分となります。その際にリクエスト番号が振られます。この番号は再レビューリクエストを行う際に必要となります。

「rbt post」でエラーが発生しましたか？慌てずに、、、。デバッグモードで再度コマンド実行しましょう。ログが大量に出ます。

rbt post -d

私が遭遇したエラーは下記のようなものでした。

>>> Got API Error 207 (HTTP code 400): The file was not found in the repository.
>>> Error data: {u'stat': u'fail', u'file': u'/XXXX/XXXXX/XXXX.js', u'err': {u'msg': u'The file was not found in the repository.', u'code': 207}, u'revision': u'12080'}
Traceback (most recent call last):
  File "C:\Program Files (x86)\RBTools\bin\..\Python27\Scripts\rbt-script.py", line 11, in 
    load_entry_point('RBTools==0.7.10', 'console_scripts', 'rbt')()
(略)
rbtools.commands.CommandError: Error validating diff

結論を言うとサーバー側の設定に問題がありました。
管理者メニューのリポジトリ設定で、パスを「svn info」で表示されるrootに設定していませんでした。1つのSVNサーバーの中に複数のプロジェクトが管理されていた場合、各プロジェクトのルートパスに設定したくなりますが、それをやるとエラーになります。嫌な仕様ですね。

Review Boardを使わないにしても、コードレビューは大切ですね！それではお疲れ様です。

参考資料

パークのソフトウエア開発者ブログ｜ICT技術（Java・Android・iPhone・C・Ruby）なら株式会社パークにお任せください

開発の解決方法や新しい手法の情報を、パークのエンジニアが提供します。パークのエンジニアが必要な場合は、ぜひお気軽にお問い合わせ下さい。株式会社パーク：http://www.pa-rk.co.jp/