2014年03月19日

コードレビューについて

このエントリーをはてなブックマークに追加
伊藤直也さんが「些末なコードレビュー」というエントリを書いて話題になっている。このエントリで伊藤さんはコードレビューの話と、はてなのJavaScriptの話と2つの話題に触れている。前者のコードレビューについてはアプレッソでは8年ほど前から「コードレビューを通っていないコードはコミット不可」というルールですべてのソースコードに対してコードレビューを必須にしてきた関係で私も思うところがあるので、エントリを書いてみようと思う。

■ 宗教論争が発生する場合にはコーディング規約の見直しを検討する
伊藤さんが例示しているように、インデントやreturnの省略などの話は好みの問題であり、議論してもソフトウェアの改善につながらない。なのでコードレビューでこうした宗教論争が起こるようなら、コーディング規約を見直すべきだ。「無駄に悩んだり議論したりすることを減らす」ことはコーディング規約の主たる効果のひとつだと言える。

コードレビューに慣れないチームが、何の考えもナシにコードレビューを始めるととにかく気になったこと大小様々な指摘が行われることになる。一見、いろいろな指摘が出て議論が活発になっているように見えるが、だいたい議論が紛糾しているのは「コードのインデント幅が違う」とか「return が省略されてる。俺は return があったほうが好み」とか「その場合は字下げをした方が綺麗にみえるんでは」とか、そんな些末なことばかりである、ということが多い。必ず一度は通る道である。
些末なコードレビューより


■ コードレビューからペアプロに柔軟に転じる
特にレビュワーが後述するリファクタリング系のコメントをする時がそうなのだが、発言に角が立つことがある。こうした時にはコードレビューから柔軟にペアプロに転じるようにする。コードレビューとペアプロには共通点も多いが、決定的に違うのは「自分がつくったコードがレビューされるか、一緒につくっていくか」という点だ。元のソースコードを改善していく時、コードレビューでは「ダメ出しを受けた」という印象になるところが、ペアプロでは「ちょっと案があるから一緒にやってみようか」という雰囲気になることが多々ある。対面でペアプロできればベストだが、リモートの場合にはSkypeなどのボイスチャットでペアプロするのもよいだろう。

もちろん、コードレビューであろうがペアプロであろうが、どう見ても改善の余地があるコード、というものも世の中にはある。そういう時にははっきりとその旨伝えなくてはならないが、言葉の選び方には留意する必要がある。例えば「ひよコード」はこうした思慮の中で生まれた言葉のひとつだ。


効率の観点からも、メールより口頭で話した方が早い、または望ましい内容があるのと同様、コードレビューでも直接話した方が良い内容があるので、この観点からもペアプロに転じた方が良い場合もある。ただし、口頭で話したがためにログにやり取りが残らないという短所もあるので、要旨については必要に応じてコミットログなど、後で参照できるところに残しておく必要がある。

■ 一見些細な内容に見えてもパフォーマンスや可読性の向上につながるものは議論の対象にする
宗教論争を避けることの補足的な話として、例えばif-then-else文の順番のように、一見趣味の問題にも思えるが、実は体感レベルのパフォーマンスに影響を与えたり、コードの可読性や誤読される危険性に大きく影響を与えるものもあるため、「些細な問題だから」とレビューのスコープから外す場合に頭の片隅に止めておく必要がある。

■ 「やってよかった」コードレビュー
社内のコードレビューガイドラインを見ながら過去の「これはやってよかった」と思えるコードレビューとしてどんなものがあったのかを思い出していたのだが、思いつくものを分類したところ次のようになった。

【リファクタリング系】
- 自分ならモデルのクラス構成をこんな風にする
- メソッド名はもっとこういう名前の方がいいかもね
- 少しクラスが担う役割が大きくなってきているから分割してみない?
- これと似た処理をしている箇所が他にあるから共通箇所を切り出してまとめてみない?
- ここスペルミスしてる

【パターン / ライブラリ系】
- この手の課題を解決するには○○パターンを適用するのもあり
- 今回自分でつくってるけど、この手の処理をやってくれる○○というライブラリがあるから今後のメンテも考えてそっち使ってみたら?
- このライブラリ、ライセンス的に大丈夫?

【バグ指摘系】
- これ、こういうケースだと正しく動作しないのでは?
- ここでこの処理をやると他の○○の箇所で問題でるんじゃない?

【パフォーマンス系】
- このやり方だと体感レベルで前より遅くならない?
- ここはこういう書き方するとだいぶ速くなるからこっちの方がいいかも

【互換性系】
- これ過去のバージョンで保存した○○という種類のデータが正しく読み込めなくなるんじゃ?

【一貫性系】
- この処理、他では違うやり方をしてて、ここだけ違うやり方だと何か理由があるんだろうと読み手に考えさせちゃうし今後やり方がどんどんバラバラになりそうだから他と合わせるか、このやり方で全部書き換えるかどっちかにしない?

【可読性系】
- この箇所が何をやっているのか理解するのにちょっと時間かかったんだけど、例えば別のこういう書き方だとサッと見てすぐ分かるようになるんじゃないかな?
- コメントが多めで、自然言語で補わないと解読しにくいコードになってるから、コメントをザクっと消して、それでも読みやすくなるようにペアプロしてみようか

■ 関連エントリ
そして、ペアプログラミングが始まる
ペアプログラミングについて
諸君 私はプログラミングが好きだ



ペアプログラミング―エンジニアとしての指南書
ローリー ウィリアムズ ロバート ケスラー
ピアソンエデュケーション
売り上げランキング: 542,486


Check このエントリーをはてなブックマークに追加
lalha at 16:59 │プログラミング  │Comments(0)TrackBack(0)

トラックバックURL

この記事にコメントする
(スパム対策のため、英数字のみからなるコメントは自動削除されますのでご注意ください。)

名前:
URL:
  情報を記憶: 評価: 顔