美しいプログラムを書く(脱添字職人編)

あらすじ

あなたはとある業務用アプリケーションの開発・保守を任されています。
このアプリケーションはC#で記述されており、
とある企業におけるプロジェクト(Project)の管理を主目的としています。
プロジェクトには何名かの社員がアサインされており(AssignedStaffs)、
プロジェクト内には必ずマネージャーが1名存在します(ManagerStaffId)。
大まかなイメージとしては以下のようなコードになっています:

public class Staff
{
    public String Id {get; set;}
    public String Name {get; set;}
    ...
}

public class Project
{
    public ArrayList AssignedStaffs {get; set;}
    public String ManagerStaffId {get; set;}
    ...

    public Project()
    {
        this.AssignedStaffs = ...;
    }
}

さて、とある事情により、プロジェクトのマネージャーの名前を使う場面があちこちで出てきました。
現状の構成ではマネージャーの名前を得ることにも一苦労しそうです。
なのでマネージャーの名前を返す Project.GetManagerName を追加することにしましょう。

public String GetManagerName()
{
    // TODO: 実装する。
    return "";
}

これは簡単なタスクです。
せっかくなので、改修作業の一連の流れを把握してもらうための例題として、
このタスクは新しく開発チームに加わったメンバーに担当してもらうことにしました。

30分後 ──

(0) 素朴な実装

新メンバーがコードレビューを要請してきました。
できあがったコードを見てみましょう:

public String GetManagerName()
{
    var i = 0;
    while (i < this.AssignedStaffs.Count)
    {
        if (((Staff)(this.AssignedStaffs[i])).Id == this.ManagerStaffId)
            break;
        i++;
    }

    var managerStaff = (Staff)(this.AssignedStaffs[i]);
    return managerStaff.Name;
}

これはリファクタリングのしがいがあるコードです。
ちょっと本腰を入れて直すことにしましょう。

(1) あちこちに散らばったキャストを無くす

AssignedStaffs の型は何故か ArrayList になっています。
ArrayList 以外にも IList などが使用されている場合もあります。
これは歴史のあるソフトウェアではよく見かける光景です。

本来ならば AssignedStaffs の型を IEnumerable<Staff> のような要素の型が明示された型に変更すべきです。
ArrayList は要素の型が不明なので、(0)のように、
要素を参照するときにその都度キャストする必要があります。
そのため、公開APIでこのような型を使うことは通常ならばありえません
(ソース: .NETのクラスライブラリ設計の8.3節)。
キャストは時と場合によっては必要になりますが、
GetManagerName のようなメソッド中のあちこちで使われると、
コードを読む際にキャストの箇所で思考が分断されてしまうため、
とてもよろしくありません(左から右へ読み進めているところに右から左へ戻らなければならない)。

それなのに何故 AssignedStaffs の型が ArrayList になっているのかというと、
このプロジェクトはジェネリックスが導入される C# 2.0 のリリースよりも早く始まったからです。
ですから AssignedStaffs が ArrayList であることを仮定しているコードは既にプロジェクト内に蔓延しており、
いますぐリファクタリングできる状況ではありません。
仕方がないので GetManagerName 内でどうにか対処することにしましょう。

幸い、C# 3.0 / .NET Framework 3.5以降であれば
Enumerable.Cast<TResult>
があります。
これを使えば ArrayList のような要素の型が明示されていないシーケンスを
IEnumerable<T> のような要素の型が明示されたシーケンスに変換することができます。
これで this.AssignedStaffs を IEnumerable<Staff> に変換すれば、
あちこちでキャストをする必要はなくなります。

ただ、 Enumerable.Cast<TResult> の戻り値は IEnumerable<TResult> になります。
IEnumerable<T> は配列のようなものを表す型なのですが、
配列のような添字参照はできません。
ですので添字参照ができるように、
Enumerable.ToList<TSource>
で戻り値を IList<Staff> に変換しています
(本当は添字参照なんてしたくないのですが、
一度に変更する量を減らしてリファクタリングの過程を分かり易くするために敢えて Enumerable.ToList<TSource> を使っています。
後述するように、最終的には Enumerable.ToList<TSource> は使わなくなります)。

なお、最初のうちは
「IEnumerable<T> なんてデータコンテナーとしては抽象的すぎて実用性ないんじゃないの」
「IEnumerable<T> を返すぐらいなら最初から配列や IList<T> のような添字参照ができる型で値を返してよ」
などと思われるかも知れませんが、
実のところ IEnumerable<T> は非常に重要な意味を持っています。
これについては後日詳細を述べることにしましょう。

以上のことから、(0)のコードは以下の形にリファクタリングできます:

public String GetManagerName()
{
    var assignedStaffs = this.AssignedStaffs.Cast<Staff>().ToList();

    var i = 0;
    while (i < assignedStaffs.Count)
    {
        if (assignedStaffs[i].Id == this.ManagerStaffId)
            break;
        i++;
    }

    var managerStaff = assignedStaffs[i];
    return managerStaff.Name;
}

(2) foreach を使って添字の管理を無くす

さて、キャストを消し去って多少はすっきりした GetManagerName のコード(1)ですが、
まだまだ読み辛いものとなっています。
その原因は i ── 添字です。

本当にやりたいことは「assignedStaffs から条件に合致する要素を見つける」ことなのですが、
(1)ではそのために添字を使って assignedStaffs の各要素を調べています。
添字を使っていると、
ループの終了条件や添字のインクリメントで単純なミスが入り込む可能性が出てくるため、
常に注意深くコードを書く羽目になってしまいます。
そして常に注意深くコードを書くことができる人は存在しません。

という訳で添字の管理を止めましょう。
添字を管理しなければしょうもないミスが入り込む余地は自然となくなります。
これには foreach を使います。

以上のことから、(1)のコードは以下の形にリファクタリングできます:

public String GetManagerName()
{
    var assignedStaffs = this.AssignedStaffs.Cast<Staff>().ToList();

    var i = 0;
    foreach (var s in assignedStaffs)
    {
        if (s.Id == this.ManagerStaffId)
            break;
        i++;
    }

    var managerStaff = assignedStaffs[i];
    return managerStaff.Name;
}

(3) インスタンスへの参照を使って添字による参照を無くす

(2)で assignedStaffs の各要素を調べるために添字を管理することを止めました。
しかし添字を使っているところがまだ残っています。
それは条件に合致した要素を後続のコードへ渡すところです。
(2)で述べたことと同様の理由で、
やはり添字を使うと不要なミスが入り込む可能性ができてしまいます。

そもそも、条件に合致した要素を伝達するために、
添字を使って間接的に行なう必然性はありません。
見つかった要素への参照を直接使えば良いのです。
また、添字の使用を止めれば Enumerable.ToList<TSource> を使う必要性もなくなります。

以上のことから、(2)のコードは以下の形にリファクタリングできます:

public String GetManagerName()
{
    var assignedStaffs = this.AssignedStaffs.Cast<Staff>();
    Staff managerStaff;

    foreach (var s in assignedStaffs)
    {
        if (s.Id == this.ManagerStaffId)
        {
            managerStaff = s;
            break;
        }
    }

    return managerStaff.Name;
}

(4) Where を使ってループを無くす

(3)で添字の管理と使用を完全に排することができました。
しかしまだ読み辛さは残っています。何故でしょうか。

それは本当にやりたいことが「条件に合致する要素を探す」ことなのに、
(3)までのコードでは本当にやりたいことを どうやるか が前面に出ているからです。
そのため、コードを見ても 何がしたいか すぐに理解できません。

例えば(2)と(3)では「 添字を使って 条件に合致する要素を探す」コードから
「添字を使って」という「どうやるか」の詳細を消し去りました。
しかしまだ改善の余地があります。
(3)の結果は「 ループを使って各要素を調べて 条件に合致する要素を探す」コードになっています。
つまり「ループを使って各要素を調べて」という「どうやるか」の詳細がまだコード上に残っているのです。

幸い、C# 3.0 / .NET Framework 3.5以降であれば
Enumerable.Where<TSource>
があります。
これを使えば IEnumerable<T> から指定した条件に合致する要素のみを取り出すことができます。
ただ、条件に合致する要素は0個かも知れませんし、1個だけかも知れませんし、30個あるかも知れません。
なので Where の戻り値は IEnumerable<T> になっています。

ですが GetManagerName では必ず条件に合致する要素が1つだけ存在すると仮定しています。
なので Where で絞り込んだ結果のうち最初の要素だけを取り出せば十分です。
これには Enumerable.First<TSource> を使います。

また、 Where は要素を絞り込むための「条件」を引数に取ります。
この「条件」はメソッドとして表現します
(なお、このようなメソッドのことを述語(predicate)と呼ぶことがあります)。
今回の場合は isManagerStaff というメソッドを作って、
それを Where の引数として渡すことにしましょう。

以上のことから、(3)のコードは以下の形にリファクタリングできます:

public String GetManagerName()
{
    var managerStaff = (
        this.AssignedStaffs
        .Cast<Staff>()
        .Where(isManagerStaff)
        .First()
    );

    return managerStaff.Name;
}

private bool isManagerStaff(Staff s)
{
    return s.Id == this.ManagerStaffId;
}

(5) ラムダ式を使ってその場でしか使われない述語メソッドの定義を無くす

さて、(4)にまで至るとコード上から「どうやるか」の詳細は消え去り、
(0)と比較するとかなり読み易くなりました。

ところが(4)では逆に問題が1つ増えてしまいました。
isManagerStaff というメソッドを定義して、
これを Where で要素を絞り込む「条件」として渡すようにしました。
しかし isManagerStaff なんてメソッドは
GetManagerName の中でしか使われません。
さらに言えば GetManagerName の中でも使用箇所は1箇所のみです。
isManagerStaff が他のメソッドからも使われる「条件」ならメソッドとして定義しておく価値はありますし、
仮に1箇所でしか使われないメソッドであっても「条件」の中味が複雑ならばメソッドとして定義して意味のある名前を付けておくことに価値はあります。
しかし isManagerStaff はどちらにも該当しません。
このようなメソッドを定義していては、
あっというまにクラスの中味が「ちょっとした」メソッドの山で埋もれてしまいます。

幸い、C# 3.0 / .NET Framework 3.5以降であれば
ラムダ式
が使えます。
ラムダ式を使えば isManagerStaff のような「ちょっとした」メソッドをその場で記述することができます。

以上のことから、(4)のコードは以下の形にリファクタリングできます:

public String GetManagerName()
{
    var managerStaff = (
        this.AssignedStaffs
        .Cast<Staff>()
        .Where(x => x.Id == this.ManagerStaffId)
        .First()
    );

    return managerStaff.Name;
}

(6) 一時変数を除去して変数の命名と管理を止める

(5)でかなり読み易いコードにリファクタリングできましたが、
まだ1点だけ問題があります。
それは managerStaff です。

(5)で述べた isManagerStaff と同様に、
この managerStaff も一度しか参照されない変数です。
managerStaff 以外にもデータを検索していたり、
managerStaff の算出過程が複雑であれば、
変数を用いて値に名前を付けることで「何がやりたいか」を黙示できます。
しかし GetManagerName の場合はどちらにも該当しませんし、
そもそも GetManagerName というメソッド名から何をしたいかや何を探しているかは自明です。

また、変数を使うということは名前を考えなければなりません。
一見すると些細なことのように思えますが、名前を考えるということは非常に重労働です。
そのような労力はもっと重要なコードに対して使うべきであって、
managerStaff のようなどうでもいい一時変数のために使うものではありません。

以上のことから、(5)のコードは以下の形にリファクタリングできます:

public String GetManagerName()
{
    return (
        this.AssignedStaffs
        .Cast<Staff>()
        .Where(x => x.Id == this.ManagerStaffId)
        .First()
        .Name
    );
}

まとめ

• 添字を使うことは止めましょう。
  今時のプログラミング言語なら foreach などの構文が用意されています。
  添字を使うと添字の管理のために不要なバグが混入する可能性が高まります。
  どうしても添字が必要なら話は別ですが、そのような機会はごく稀です。
• 「何をしたいか」を基準にプログラムを書きましょう。
  今時のプログラミング言語なら Where などのライブラリ関数が用意されています。
  foreach のような明示的なループは「どうやるか」が前面に出ている可能性が高いです。
  どうしてもループが必要な場合もありますが、そのような機会は稀です。
  大抵は Where のようなライブラリ関数で対応できます。
• 一時変数の多用は控えましょう。
  変数が多いほど、各変数がどのような意図で使われているかを読み解く作業が増え、
  後でコードを読むときに苦労することになります。
  意図が伝わり易い変数名にすれば読む際の負担は減るものの、
  そのような苦労は重要な変数に対して行うべきで、
  どうでもいい一時変数にまで考える必要はありません。
  適切な単位に関数を分け関数名を工夫していれば、
  一時変数を多用する機会はまずありません。

(0)のようなコードは古式ゆかしいものです。
私も昔は(0)のようなコードを量産していたものです。
しかし量産していくうちに嫌気が差してきました。
注意深く書かなければ簡単に動かないコードができあがるし、
苦労して書いたところでちっとも読み易くないからです。
徐々にレベルアップしていき、今では最初から(6)のようなコードを書いています。

プログラミングそのものに慣れていないうちはいきなり(6)のようなコードを書くことは難しいかと思います。
何故なら、上述の通り、(0)から(6)に至るまでには様々な壁があるからです。
初めのうちは(0)でも構いませんが、徐々にレベルアップして(6)をすらすら書けるようになりましょう。
その方が色々と幸せになれます。

次回は「美しいプログラムを書く(リスト処理ライブラリ編)」です。