読みやすいコード

前置き

「リーダブルコード」という本を読んだとき、読みやすいコードの書き方を人に教えるのは、絵画やクラッシック音楽の鑑賞方法を教えるようなものだと思い至りました。つまり、分かる人には教えなくても分かるけど、分からない人には教えても分からないのです。修得できる技術ではありますが、教え込めるものではなく、本人が目覚めるのを待つしかない気がします。

なぜでしょうか。一言で言えば、ソフトウェア開発が変化に富んでいるせいでしょう。コードを読みやすくするための定石や原則、教訓などを挙げることはできるでしょうが、それらには例外が付き物です。どんなルールにも、それを破るべき状況があります。しかし、守るべきルールだけでなく、それを破るべき状況まで教えようとした途端に、ルールの力強さが失われてしまいます。何しろ、相反する主張を同時にすることになりますからね。

守るべきルールを教える。他人が手助けできるのは、ここまでなのでしょう。守破離の守。破と離は本人任せで…。

読みにくいコード

コードレビューのレビューアの立場で、読みにくいコードの特徴を挙げてみます。これらの特徴を避けて書けば、読みやすいコードになるはずですね。

メソッドや関数のような狭い範囲から始め、ボトムアップで視野を広げながら考察します。

読みにくいメソッド/関数

まずは、定量的な、あるいは見た目に関する特徴から挙げます。これらは、割と客観的に評価できるでしょう。

定量的特徴
  • 長い ~ 例えば、100行を超える
  • 詰まっている ~ 空行なしで30行以上
  • 1行が長い ~ 半角100文字以上
  • 変数が多い ~ 10個とか
  • インデントが深い ~ 10段超
  • コメントが目障り ~ コード1行ごとにコメント1行とか

続いて、そのメソッド/関数を使う立場から見た特徴です。外部仕様があいまいなメソッド/関数は使いにくいですし、そういったメソッド/関数を使って書かれたコードは読みにくくなります。

外部仕様
  • 有効な引数の範囲があいまい ~ 上限/下限はあるのか、NULLを許すかどうかなど
  • 入力引数なのか、出力引数なのかがあいまい
  • 異常時や準正常時のreturn値があいまい
  • 例外を投げるかどうかがあいまい
  • 副作用の内容があいまい

副作用とは、「return値や出力引数によって値を返す」以外の作用を指します。例えば、キーボード入力を待つ、何かをファイルに書く、メンバ変数やグローバル変数を変更するなどです。値を返すことではなく副作用自体を目的としたメソッド/関数も多いと思いますが、そういったメソッド/関数では副作用を明確にすることが特に重要です(正にそれが外部仕様ですから)。

最後に、読みにくいメソッド/関数の、定性的な(あるいはコードの本質に関する)特徴を挙げます。主観に左右されるので、グレイゾーンが広いです。

定性的特徴
  • 関数名から機能が推測できない、あるいは、似たような名前の別の関数との違いが分からない
  • 変数名から役割が推測できない、あるいは、似たような名前の別の変数との違いが分からない
  • 名前から、その素性が推測できない
  • アルゴリズムが素直でない
  • 不相応な副作用を持つ

「素性」というのは、それが何者かということです。例えば…。

  • 変数か定数か関数か
  • スコープの広さ(影響範囲)
  • 自分が定義したものか、ライブラリ/フレームワーク/OSが提供するものか

後述しますが、名前から素性が推測できるように名前の表記を工夫すると、コードが読みやすくなります。

読みにくいクラス/ファイル

少し視野を広げ、クラスや1つのソースファイルに関して、読みにくさの特徴を挙げます。

定量的特徴
  • メソッド/関数が多い ~ 30くらい
  • メンバ変数/static変数が多い ~ 30個
  • 使われてないメソッド/関数を持つ
  • 必要性を考えずに、getter/setterを定義している
外部仕様
  • 準備→利用→廃棄の手順が分かりにくい
定性的特徴
  • クラス名から、その機能が推測できない

読みにくいコード(全体)

更に視野を広げ、プログラム全体を見てみます。ここまで来ると、抽象的過ぎて、自分のコードがこの特徴に当てはまるかどうか判断できないかもしれません。

定性的特徴
  • ネーミングに一貫性が無い
  • クラス間の役割分担が不適切
  • パッケージ/名前空間/モジュールなどを使って、適度にグループ化されてない

コードを読みやすくするヒント

バグを防ぐためにダサいスタイルを強要するより、開発者のスキルを上げろ

パッと思いつく例は、C言語のコーディング規約に良く見られる、次の2つです。

BAD PRACTICES
  • 条件式では、等式の左辺に定数を書け
  • if文では、単文でも{}をつけろ
if ( BAD_CODE == code ) {
    return EBADCODE;
}

これらは、未熟なプログラマが以下のように書いてしまうのを防ぐのが狙いです。

if ( code = BAD_CODE )

比較じゃなくて代入になってますね。さらにデバッグ用のprintfを仕込もうとして、次のようにやってしまいます。

if ( code = BAD_CODE )
    printf("ERROR: code(%d) is bad.", code);
    return EBADCODE;

こういう失敗は私にも経験がありますが、それもまた勉強になります。痛い目にあってスキルアップすればよいと思います。私の好みはこっちです。

添削後
if ( code == BAD_CODE ) return EBADCODE;

特に、単文でreturn/break/continueするだけなら、{}無しの1行で書いた方が読みやすいです。

また、ある職場でのエピソードですが、C言語の次のコードが他の人には理解できないと言われました。

unsigned char* buf = malloc(100);
if ( !buf ) {    // 失敗?
  ...

なんと、その職場の多くのプログラマは、! を知らなかったのです。たちまち、コーディングスタイルに「!ではなく、==か!=を使え」の一文が追加されてしまいました。

if ( buf == NULL ) {

チームの方針なら従いますけどね。やれやれ、といったところです。

リファクタリングを待たずに、常にクリーンに保て

「後でまとめてリファクタリング」という発想は危険です。リファクタリングは、技術力の低いリーダー層が飛びつきがちなpracticeですが、これには、まともな自動テスト環境の存在が(あるいは、若手社員で構成された人力テスト部隊と、まともなテスト仕様書の存在が)大前提となります。残念ながら、そんなものは滅多に存在しません。

既存のコードを修正するとき、たぶん、さすがに、そのときはテストもしますよね。そして、それが最初で最後のテストになることも珍しくないでしょう。つまり、そこがコードをきれいに保つラストチャンスなのです。そのタイミングでバグが出るのは自然なので、ある程度なら思い切った改造も許されると思います。

ごく基本的なことですが、

  • コードの追加により関数がかなり長くなるなら、別関数に分ける
  • 変数の追加により既存の変数名との混乱が生じるなら、既存変数のリネームも検討する

こんなことを、面倒くさがらずにやりましょう。

ネーミングのヒント

表記のバリエーション

大文字と小文字とアンダースコア(_)の使い方次第で、同じ名前にもバリエーションを持たせることができます。

規則
tmp, strlen小文字のみ。単語区切りなし。
_tmp, _strlenアンダースコアをprefixする。
tmp_, x_アンダースコアをsuffixする。
file_pathアンダースコアで単語を区切る。
filePath大文字で単語を区切る(キャメルケース)。
FilePath先頭も大文字(パスカルケース)。
MAX, EBADCODE大文字のみ。単語区切りなし。
FILE_PATHアンダースコアで単語を区切る。

その他に、特定の文字をprefixする手もありますね。クラス名はCで始めるとか、インスタンス変数にはmをprefixするとか。

表記に意味を込める

例えば、スコープの広さを表記で区別します。小文字より大文字の方が、また、短い名前より長い名前の方が目立ちますよね。スコープの広い識別子は影響力も強いので、目立つ名前をつけましょう。私がC/C++時代に使っていたのは、以下のようなルールです。

  • ローカル変数や仮引数は、小文字の短い名前(i,j,k,buf,tmpなどでもOK)
  • インスタンス変数には、m_をprefixし(MFCの真似)、単語区切りはアンダースコア
  • static変数やstatic関数には、アンダースコアをprefixし、単語区切りはアンダースコア
  • グローバル変数は使わない(どうしても使うならパスカルケース)

マイクロソフトのハンガリアン記法には賛否両論ありますが、個人的に、ポインタ変数にpをprefixするのは賛成です。さらに進んで、freeする責任のあるポインタにはpを、(他所からもらった)参照するだけのポインタにはrpをprefixする(rはreferentialの意味)というのはどうでしょう? メモリリーク対策の一助として。

void foo(char* rpfname)
{
    char* pfname = malloc(strlen(rpfname) + 1);
    strcpy(pfname, rpfname);
    ...
    ...
    free(pfname);
}

他にも、以下のような工夫が考えられます。

■定数か変数か
大文字のみなら定数、小文字か大小混在なら変数。
■(C/C++で)マクロか関数か
大文字のみならマクロ、小文字か大小混在なら関数。

自分のスタイルを決め、ネーミングに一貫性を出す

プログラマは常に、ネーミングという行為を通して決断力を試されています。このプレッシャーがストレスになる可能性もあるでしょう。自分のスタイルを決めておくと、少しだけプレッシャーが軽くなるかもしれません。例を挙げます。

■配列には複数形
filesとかrequestsとか。bufは例外。またfileListとかrequestArrayなら、listやarrayが「複数」を示唆するので複数形にする必要は無い。tableも。
■「~の数」には、num + 複数形
例えば「ファイル数」ならnumFilesとか。英語のnumber of filesを縮めた感じ。
■範囲
上限値と下限値にはminとmaxか、topとbottom、先頭と最後にはheadとtailか、beginとend。beginとendの場合、endは最後尾の1つあと(つまり範囲の外)を指します。
■配列のインデックス
ixと略す。暗黙で0オリジン。
■「~番号」
noと略す。暗黙で1オリジン。
■単位を名前に埋め込む
例えばelapse_msecとすればミリ秒単位の経過時間だし、len_in_byteならバイト単位の文字列長だと分かる。
■文字コードを名前に埋め込む
文字列を格納するバイト配列なら、str_utf8とか、content_sjisとか。
■加工状態を名前に埋め込む
raw, plain, encrypted, escaped, sanitized, encodedなどをprefix/suffixする。

意味が幅広い単語を避ける

コードはもちろん、コメントの日本語にも気を使いましょう。

stop
一時停止なのか完全停止なのか読み取れません。一時停止ならpauseやsuspend、完全停止ならcancel、terminate、kill、destroyなどはどうでしょう。
check
英語の苦手な人に乱用される傾向があります。例えばcheckFileではファイルの何をチェックするのか分かりません。
~中
例えば「停止中」という状態名は、停止した状態なのか、停止処理の最中なのか迷います。
設定する
これも乱用されます。単なる代入から、複雑なIOを伴う処理まで、幅広く使えるので、真意が伝わりにくいです。

以下は、genericな単語とspecificな類語の対応です(「リーダブルコード」から抜粋)。

send
deliver, dispatch, announce, distribute, route
find
search, extract, locate, recover
start
launch, create, begin, open
make
create, setup, build, generate, compose, add, new

特殊すぎる(ネイティブな人でも滅多に使わないような)単語を使うのも避けた方が良いと思います。変数名を決めるとき和英辞書に頼っている人は、かなり特殊な単語を使ってしまう危険があるので気をつけましょう(和英で調べて、英和で再調査するとか)。

設計上のヒント

ネストを浅く保つ、あるいは例外的なケースの処理を先に書く

妙なタイトルですが、ネストの深さと、コードを書く順番は密接に関連しており、コードの読みやすさに影響します。

BAD
for (;;) {
  if ( !EndFlag ) {     // 終了フラグが立ってない?
    Message* pmsg = GetMessage();
    if ( pmsg ) {       // メッセージあり?
      // メッセージを処理。
      ...
      ...
      ...
    }
    else {              // メッセージなし。
      Sleep(1);
    }
  }
}

上記の例では、ネストは3段です。3段くらいなら問題になりませんが、ネストに無頓着な人なら、どんどんエスカレートしていきます。ネストはプログラムの構造を反映しており、ネストが深いということは、それだけコードが複雑になっているということです。

また、このコードの最初のif文にはelseが無いのですが、読み手は、最後の方まで読んでやっとそのことに気付きます。それまでは、ずっとelseの存在を念頭に置いてコードを読まなければなりません。2番目のif文にはelseがありますが、その処理は短いので、条件式を逆にして先に「メッセージなし」の処理を片付けた方が、一番大事な「メッセージの処理」にじっくり取り組めるでしょう。

例外的なケースの処理を先に書き、break/continue/returnなどで早めに逃がす(はじく)ことにより、ネストを節約することができます。

GOOD
for (;;) {
  if ( EndFlag ) break;    // 終了要求あり?

  Message* pmsg = GetMessage();
  if ( !pmsg ) {           // メッセージなし?
    Sleep(1);
    continue;
  }

  // メッセージを処理。
  ...
  ...
  ...
}

ネストが2段になりました。たった1段の違いですが、段数の削減以外にも効果があります。それは、主たる処理(この例ではメッセージを処理する部分)のコードが3段目から1段目に移動しているということです。主たる処理は桁数も長くなりがちなので、インデントの浅い位置に書きたいですよね。

またネストの深さに無頓着な人のコードには、以下のような悪循環が見られます。

ネストが深い
  →インデントが大きい
    →右にはみ出るのを避けるため余分な改行を入れる
      →行数が増える
    →どうしても右にはみ出だす
      →コードの右側にコメントが書けない
        →行数が増える
        →書くべきコメントを省略してしまう

関数は短く

DOS時代の名残で、いまだに「関数は24行以内」なんていうコーディング規約を使っているチームもありますが、さすがにそれは無茶でしょう。でも、スクロールせずに関数全体を見渡せる方が読みやすいことは確かです。多少はスクロールしたとして、100行くらいが上限だと思います。

長い関数は、複数のサブ関数に分割しましょう。例えば200行の関数を書いたとして、どこで分割すれば良いか分からないとしたら、たぶんあなたは、その200行のコード自体を理解/掌握できていません。そもそも考察が足りないのです。

インスタンス変数を安易に使わない

グローバル変数が悪だというのはほぼ常識化していると思いますが、インスタンス変数はどうでしょうか。インスタンス変数のスコープはクラスのインスタンスメソッド全体なので、結構広いです。スコープが広いということは、コードを変更したり解析するときに、ケアすべき範囲が広いということです。なので、インスタンス変数の使用は必要最小限に留めましょう。

特に、メソッド間で共有したいデータをインスタンス変数にする、という発想は誤りです。インスタンス変数は、インスタンスの属性を保持するために使うべきです。属性とは、そのインスタンスの生存期間中ずっとついて回るような性質/状態のことです。

悪い例
class Parser
  def parse(doc)
    preparse(doc)
    parse_core
  end

  private
  def preparse(doc)
    ...
    @preparsed_doc = ...
  end
  def parse_core
    if @preparsed_doc
      ...
    end
  end
end

rubyのコードです。@preparsed_docがインスタンス変数で、パースの途中経過を格納しています。これはParserインスタンスの属性としてふさわしくないでしょう。parse_coreの引数にすれば、インスタンス変数は不要です。

添削後
class Parser
  def parse(doc)
    ppdoc = preparse(doc)
    parse_core(ppdoc)
  end

  private
  def preparse(doc)
    ...
    ppdoc = ...
    return ppdoc
  end
  def parse_core(ppdoc)
    if ppdoc
      ...
    end
  end
end

ローカル変数といえども乱用はよくない

ローカル変数ならスコープは狭いので使い放題かというと、そうでもありません。例えば100行のメソッドなら、ローカル変数のスコープも100行です。たとえ実際は先頭の3行で使っているだけの変数でも、コードの読み手は、残り97行を読む間ずっとその変数の存在を意識します。

以下のような対策が考えられます。

  • メソッド/関数自体を短くする
  • 変数を使わずに実装する
  • ブロックや別メソッド/関数にくくり出してスコープを狭める

レイアウト

コメントや代入式のインデント

これって、どうですか?

abc      = val;     /* blah blah blah      */
abcdef   = val2;    /* hoge hoge hoge hoge */

等号の位置、コメントの開始位置、及びコメントの終了位置を桁揃えしています。メンテしにくいので、私なら、コメントの開始位置を合わせるくらいで妥協します。

abc = val;        // blah blah blah
abcdef = val2;    // hoge hoge hoge hoge

コメントを、対象のどちら側につけるか

次のコードにコメントをつけてみましょう。

  FILEHANDLE* pfh = OpenFile("foo.txt", FOR_READ);
  if ( !pfh ) {
    ...
  }
  ...

コードの上側にコメントを入れるのは、コードを読む前にコメントを読ませたい場合や、数行のコードのかたまりに対してコメントを入れる場合です。

  // ファイルを読んで画面に出す。
  // ただしファイルが開けない場合は、……。
  FILEHANDLE* pfh = OpenFile("foo.txt", FOR_READ);
  if ( !pfh ) {
    ...
  }
  ...

コードを先に読ませて、それを補足するコメントを付け足したいなら、右側につけます。

  FILEHANDLE* pfh = OpenFile("foo.txt", FOR_READ);  // 失敗したらNULLを返す。
  if ( !pfh ) {      // 失敗?
    ...
  }
  ...

私の場合、右側にスペースが足りない場合は、下側に、インデントを下げてコメントを入れます。

  FILEHANDLE* pfh = OpenFile("foo.txt", FOR_READ);
                       // デフォルトでテキストモード。
                       // 失敗したらNULL。

「リーダブルコード」には、対象の左側にコメントをつけるという案も載ってました。

  FILEHANDLE* pfh = OpenFile("foo.txt", FOR_READ, /* isBinMode */true);

1行は80桁以内

「1行は80桁以内」というのはDOS時代からの慣習ですが、今でも通用すると思います。これくらいを目安にしておくと、2つのソースファイルを左右に並べても何とか読めます。

古い習慣

昔はかたくなに守っていた習慣で、今は捨ててしまったものを挙げます。

■メソッド/関数の開始の { は行頭
Java時代になると、無駄な抵抗ですね。
■インデントはTABで、TAB幅は4
rubyに惚れて以来、どうでも良くなりました。
■コメント行の前には空行を空ける
適度な隙間か省スペースか…。半行空けられるといいんだけど。
Last modified:2012/11/14 12:47:25
Keyword(s):
References:[組み込みソフト開発101]
This page is frozen.