こんにちは。キャスレーコンサルティングID(インテグレーション＆サービス)部の秦です。
IT業界で長く働いていると、いつかは他人の書いたソースコードをレビューする機会があります。
私が新人の頃には自身のコードをレビューや、諸先輩方よりアドバイスを頂いて参りました。
また、現在ではチームメンバーのソースコードをレビューをする機会があります。
そのため、今回は
「美しいソースコードを残すためにレビュアーとして気を付けている10のこと」をご紹介したいと思います。
本記事での「美しい」は、私自身の主観的な部分や好みも含んでいるため、これが正解というわけではありません。
あくまでもひとつの参考として、ご覧頂ければ幸いです。
また、「ソースコードのレビューって、何をすれば良いの？」
「どこをどうチェックすれば良いのか、わからない」
「他の人はどんな点に着目して、レビューをしているのだろう？」
そんなレビュアーの皆様のご参考になれば、幸いです。
尚、本記事の各ソースコード例では、PHPを使わせて頂きます。
一. コーディング規約を確認せよ
現場に入ってコーディングを行う際に、その現場の「コーディング規約」が設けられている場合があります。
このコーディング規約が、何のために設けられているか。
それは、各個人のソースコードのバラつきを防ぎ、コードの書き方を統一することで、
「ソースコードの可読性や保守性を維持すること」を目的としていることが、主であると思います。
また、バグの予防という目的もあります。他にも色々な目的があるかと思います。
各メンバーがコーディングを行う際、レビュアーがコードレビューを行う際、
「コーディング規約に従って書かれているか」を意識しながら作業を行います。
コーディング規約があればそれに従いますが、そんなものを作っている暇がない、という現場も多々あるかと思いますので、
その場合は、レビュアーがコーディング規約の役目を担って、レビューを行う必要があるでしょう。
そうなんです、レビュアーはソースコードの品質を保つためにとても大事な役割を担っているのです。
従って、先ずはその現場のコーディング規約やガイドラインの有無を確認しましょう。
設けられている場合には規約に従ってコーディングが行われているか、をチェックしましょう。
二. インデントを確認せよ
「インデント」とは文章の行頭に挿入する「空白」のことです。
プログラミング言語には、if文やfor文などの制御構文と呼ばれるものがありますが、
ブロック構造をわかりやすくするために、インデントを用います。
以下の例2-1と例2-2を、比較してみてください。
例2-1. インデントなし
class Hoge
{
public function foo($arg1)
{
if ($arg1 == "") {
return false;
}
return $arg1;
}
}
例2-2. インデントあり
class Hoge
{
public function foo($arg1)
{
if ($arg1 == "") {
return false;
}
return $arg1;
}
}
全く読み易さが違いますよね。
インデントは、コーディングをする者にとっては常識的なものですが、
これが意外と統一されていない場合が多いんです。
（最近の言語によっては、インデント自体が構文である場合もありますね）
人によって、EclipseといったIDE(統合開発環境)や、様々なエディタを使ってコーディングを行うことでしょう。
エディタ毎にインデントの設定は異なりますし、フォーマッターが付いているような高性能なエディタですと
一律で同じインデントの種類（タブ、半角スペース）を、入れてくれるような場合もあります。
タブを入れた場合と半角スペースで入れた場合ではエディタでの表示も変わって来ます。
これによってインデントが崩れてしまい、可読性が下がるといったことは稀ではありません。
例2-3. タブとスペースが混在していると…
class Hoge
{
public function foo($arg1)
{
if ($arg1 == "") { //タブ
return false; //タブ
} //タブ
return $arg1; //半角スペース
}
}
どの行が同じブロックなのかが、分かりにくくなりますよね。
コーディング前に気が付けるのが一番良いのですが、レビュー時には必ず確認しましょう。
三. スペースの位置を確認せよ
多くのコードを読んで来て気付いたのが、人によってコードの書き方に癖がある、ということです。
例えば、
例3-1. Aさんの書き方
for(var $i=0;$i<10;$i++){
echo $i;
}
例3-2. Bさんの書き方
for (var $i=0; $i<10; $i++){
echo $i;
}
例3-3. Cさんの書き方
for (var $i = 0; $i < 10; $i++) {
echo $i;
}
for文ひとつ取ってみても、これだけの違いが生じます。
傾向として、要員の入れ替わりが多くレビューが疎かになりがちなプロジェクト程、この事象は悪化するように思います。
かなり細かい話ではありますが、これは広く配布されているコーディングガイドにも記載されている項目です。
例えばPHPですと、「PSR-2」というコーディング規約が存在します。
こちらにもインデントの種類やスペースの入れ方など、細かいところへの言及が記載されております。
弊社でもPHP開発プロジェクトはいくつかございますが、このPSR-2は良く使わせて頂いております。
どれが正解ということはありませんので、どんな書き方でも良いのですが、
美しいコードを残すためにも、メンバー間で統一されるのが良いかと思います。
四. 改行の位置を確認せよ
はい、またまた細かい話ですが、改行の位置も人によって癖があります。
例4-1. Aさんの書き方
public function hoge()
{
for (var $i=0; $i<10; $i++)
{
$a = $b = $i;
}
}
例4-2. Bさんの書き方
public function hoge() {
for (var $i=0; $i<10; $i++) {
$a = $i;
$b = $i;
}
}
functionとforの末尾の括弧が異なってますね。
これらも統一されるのが良いかと思います。
ちなみにですが、上の例では変数$aと$bについて、代入の方法を変えています。
実はこれ、実行速度に差が出る書き方なんです。
例え0.001sec単位の違いでも、塵も積もれば山となります。
レビュアーはパフォーマンスの面についてもチェックされると良いかと思います。
五. ハードコーディングを確認せよ
時間の迫られるシステム開発で良く散見されるのが、ハードコーディングです。
例えば、数量と単価と消費税率から、税込みの金額を計算する処理があるとしましょう。
例5-1. 金額の計算処理
public function calcAmount($quantity, $unitPrice)
{
retrun $quantity * $unitPrice * (1 + 0.08);
}
さて、本執筆2018年3月時点での消費税率は8%ですが、将来的に税率が変わったらどうなるでしょう。
システム内での金額計算が上記の関数ひとつだけであれば、それほど影響はないかもしれません。
しかし、消費税率を扱う計算が他にいくつも存在した場合、影響箇所は広がってしまうことでしょう。
そのような状況を避けるためにも、ハードコーディングは出来る限り控えるべきと考えます。
下記例5-2のように定数にするか、あるいはDBに税率マスタとして設けるか、
方法は色々あるかと思いますが、今回は定数を使うことにします。
例5-2. 金額の計算処理
private const TAX = 0.08;
public function calcAmount($quantity, $unitPrice)
{
retrun $quantity * $unitPrice * (1 + self::TAX);
}
これで税率に変更があった場合には定数の値を変えるだけで済みますね。
六. 誤字脱字・スペルミスを確認せよ
結構な頻度でありがちなのが、英単語のスペルミスです。
現代でプログラミングを行う者達にとって、英語は必須と言っても過言ではないスキルですが、
変数名や定数名のスペルミスは良く見かけられます。
「HTMLにclass属性を付与してスタイルを当てたものの、何故かスタイルが当たってくれない…」
「連想配列に値を入れたものの、何故かnullが入ってしまっている…」
疑うべき原因のひとつは、スペルミスです。
ある人は正確なスペルで書いたとします。またある人はスペルミスをしてしまったとします。
これが乱立すると、あちこちで複数のスペルを孕んだ単語が混在してしまい、バグの温床にもなります。
最近はあまり残さないかもしれないですが、@authorなど作成者の名前を書いた際、
後からスペルミスが発見されると、なかなか恥ずかしいものです。
レビュアーも常に誤字脱字、スペルミスのチェックには気を配りましょう。
七. コメントを確認せよ
納期が短く、急いで開発しなければならない…そんな状況は良くあることですが、
どうしてもコメントが疎かになりがちです。
熟練したエンジニアであれば、コメントが無くともソースコードを読むことは難しいことではないでしょう。
しかし、書いているのは他人。自分ですらも時間が経ってから読んだコードは、読むのに時間がかかるものです。
JavaDocsのように、APIドキュメントとして起こせるようなコメントも然りですが、
各処理の説明を行うためのコメントも、非常に重要なものです。
レビュアーが読んでパッとわからないと感じたコードは、他の人が読んでもわからないと思っても良いでしょう。
1行でも多くコメントが書いてあることで、レビュー速度も上がりますし、正確度も上がります。
もちろんコメントと処理の内容が合っていない場合もあるため、鵜呑みにはできませんが、無いよりマシです。
レビュアーはコメントがしっかり書かれているか、誰が読んでもパッと見で理解できるか、チェックしましょう。
八. ネストの深さを確認せよ
処理によっては避けられない場合もあるかと思いますが、ネストは出来るだけ深くならないよう注意しましょう。
極端な例で恐縮ですが、以下のようなコードです。
例8-1. ネストが深いコード
public function nested($a, $b, $c, $d)
{
if ($a == 1) {
if ($b == 1) {
if ($c == 1) {
if ($d == 1) {
$e = true;
}
}
}
}
}
例8-2. ネストが最低限のコード
public function nested($a, $b, $c, $d)
{
if ($a == 1 && $b == 1 && $c == 1 && $d == 1) {
$e = true;
}
}
エディタで編集すると良く実感するのですが、ネストが深いと開始と終了の位置を追うのが大変なことが度々あります。
if文の条件をまとめる、処理を外出しにする等の工夫をすると良いかと思います。
九. 命名を確認せよ
クラス名、変数名、定数名、関数名…プログラムには名前を付けられるものが多く存在します。
「この関数名は処理を的確に表しているか」
「この変数名は用途を的確に表しているか」
などなど、名前を付ける以上は判別できるようにするべきと考えます。
以前の現場で実際にあったことですが、
private const ONE = 1; private const TWO = 2; private const THREE = 3;
上記のようなコードを見かけたことがあります。
確かに一目瞭然ではあるのですが…何に使ったら良いのか全くもって不明ですよね。
private const FLAG_OFF = 0; // フラグON private const FLAG_ON = 1; // フラグOFF private const TAX = 0.08; // 消費税率
といったように、折角名前を付けてあげるので、意味のある名前を付けてあげましょう！
十. 処理の長さ、冗長性を確認せよ
ひとつの関数の中には、いくらでも処理を書くことができます。
何万行でも、です。
そんなコードは読みたくありませんよね。日が暮れてしまいます。
私は「ひとつの関数ではひとつの目的を」が良いと考えています。
共通化出来る処理はないか、外出し出来る処理はないか、
ループで回せる処理はないか、など「冗長性の排除」を行うのもひとつのポイントです。
少し極端ですが、以下の例をご覧ください。
例10-1. 冗長的なコード
private const TAX = 0.08;
/**
* 数量と単価の配列から金額を計算
*/
public function calcAmounts($ary)
{
$amounts = [];
// 数量を取得
$quantity = $ary[0]['quantity'];
// 単価を取得
$unitPrice = $ary[0]['unitPrice'];
// 金額を計算
$amounts[0] = $quantity * $unitPrice * (1 + self::TAX);
// 数量を取得
$quantity = $ary[1]['quantity'];
// 単価を取得
$unitPrice = $ary[1]['unitPrice'];
// 金額を計算
$amounts[1] = $quantity * $unitPrice * (1 + self::TAX);
// 数量を取得
$quantity = $ary[2]['quantity'];
// 単価を取得
$unitPrice = $ary[2]['unitPrice'];
// 金額を計算
$amounts[2] = $quantity * $unitPrice * (1 + self::TAX);
// 金額の配列を返却
return $amounts;
}
例10-2. 冗長性を排除したコード
private const TAX = 0.08;
/**
* 数量と単価の配列から金額を計算
*/
public function calcAmounts($ary)
{
$amounts = [];
foreach ($ary as $val) {
// 数量を取得
$quantity = $val['quantity'];
// 単価を取得
$unitPrice = $val['unitPrice'];
// 金額(税込み)を計算し格納
$amounts[] = $this->calcAmount($quantity, $unitPrice, self::TAX);
}
// 金額の配列を返却
return $amounts;
}
/**
* 金額を計算
*/
public function calcAmount($quantity, $unitPrice, $tax)
{
// 金額 = 数量 * 単価 * (1 + 税率(%))
return $quantity * $unitPrice * (1 + $tax);
}
いかがでしょうか。冗長性が排除され見通しが良くなっているのがおわかりでしょうか。
金額計算も関数化され、他での使い回しが利くようにもなりましたね。
ループで回せるものは回す、関数化できるもの・すべきものは関数化する。
これも塵も積もれば山となるです。
おわりに
今回は、ソースコードの体裁を意識したレビューのポイントについて、ご紹介しました。
プログラムの正しさのレビューについては今回は言及してませんが、また機会があればご紹介したいと思います。
書く方も読む方も、日頃から気を付けてコーディングを行ってくださいね。
それではまた。
最後までご覧頂き、ありがとうございました。
