10月に入社した開発部の澤井です。現在、新人研修を受けています。カルテット開発部では、新人一人に対して教育担当者が二人もつきます。（とても贅沢です）

ところで、皆さんはコード設計についてどのような考えをお持ちでしょうか。私は現在、新人研修を通してより良いコード設計について学ぶ日々ですが、残念ながら「これが良いコード設計です」と言えるだけの知識はまだありません。

そこで本エントリーでは、新人研修で取り組んだ問題を例にして、実際に自分のコードがどのように変化していったのかを書くことで、私が新人研修を通して学んだこと、考えたことをお伝えできればと思います。

取り組んだ問題：「群島の宝探し」

これは、教育担当の後藤が作成したオリジナル問題です。大まかなルールは以下のとおりです。

  • 群島はスタートとゴール、およびA島〜E島の5つの島からなる
  • プレイヤーは1回〜複数回サイコロを振り、スタートからゴールを目指す
  • プレイヤーのいる島ごとに、サイコロの出目に応じて決められた行動が発生する
  • 行動は、「ゴールドの獲得」と「次の島への移動」からなる（サイコロの出目と行動のセットを「ヒント」と呼ぶ）
  • ゴールするには、例えば「500ゴールド以上所持していること」など、一定以上の獲得ゴールドが必要となる

ヒントの例

例えば、下記のようなヒントがあります。

  • 出目が1ならA島へ
  • 出目が奇数なら200ゴールド獲得してB島へ
  • 出目が偶数なら100ゴールド獲得してC島へ
  • 出目が6かつ所持ゴールドが500ゴールド以上ならゴールへ
  • 当てはまるヒントがなければスタートへ

本エントリーでは、島とヒントを扱う部分に注目します。

最初のコード

私が考えた最初のコードは、下記のようなものでした。

島とヒントを同じクラスで表現すると複雑になるので、それらを別々のクラスに分けました。島を表すクラスをIslandクラス、ヒントを表すクラスをHintクラスと名付けました。Islandクラスは、その島におけるヒントのリスト（Hintのインスタンスの配列）を保持しています。

Hintクラス

<?php
class Hint
{
    private $requiredGold;

    private $nextIsland;

    private $acquiredGold;

    // ... 

    public function getNext(): array
    {
        return [
            'nextIsland' => $this->nextIsland,
            'acquiredGold' => $this->acquiredGold,
        ];
    }
}

Islandクラス

<?php
class Island
{
    private $hints = [];
    
    public function __construct(array $rules)
    {
        foreach ($rules as $condition => $rule) {
            $this->hints[$condition] = new Hint($rule);
        }
    }
    
    public function getNext(int $numberOfDice, int $goldTotal): array
    {
        foreach ($this->hints as $condition => $hint) {
            $requiredGold = $hint->getRequiredGold();
            if (is_numeric($condition) === true) {
                if ($condition === $numberOfDice && $goldTotal >= $requiredGold) {
                    return $hint->getNext();
                };
            }
            if ($condition === 'odd') {
                if ($numberOfDice % 2 === 1 && $goldTotal >= $requiredGold) {
                    return $hint->getNext();
                }
            }
            if ($condition === 'even') {
                if ($numberOfDice % 2 === 0 && $goldTotal >= $requiredGold) {
                    return $hint->getNext();
                }
            }
            if ($condition === 'other' && $goldTotal >= $requiredGold) {
                return $hint->getNext();
            }
        };

        throw new \Exception('当てはまるヒントがありません');
    }
}

みなさんは、上記のコードを見てどのような印象を受けたでしょうか？

私は、レビューを受けた時点では、IslandクラスとHintクラスを分けることで、それぞれの役割がはっきりし、コードがスッキリしたと考えていました。

Island::getNext()が、自らが保持しているヒントをもとに次に向かうべき島や取得できるゴールドといった情報を導く様子を、上手く表現できていると考えていました。

しかしここで教育担当者から以下のような指摘を受けました。

OKと言いたいところですが、このgetNext()のコードと、Hintクラスを眺めて、コードをキレイに整理できないか考えて頂きたいです。

最初のコードでは、IslandクラスのgetNext()にis_numeric($condition) === trueのような処理が含まれていましたが、後から考えると、これはHintクラスに持たせたほうが自然な処理であり、IslandクラスとHintクラスの役割分担ができていませんでした。

クラスの役割まとめ

問題点を整理するために、改善前のクラスの役割をまとめます。

Hintクラス

  • ゴールに必要なゴールド、進む島名、獲得ゴールドをプロパティとして持つ
  • getNext()は保持している進む島名、獲得ゴールドを返す

Islandクラス

  • ヒントごとに、Hintクラスのインスタンスを作成し配列で保持する
  • getNext()は出目の和と総ゴールドを受け取り、「数字」・「奇数」・「偶数」・「その他」の条件判定を行い、保持しているhintインスタンスのgetNext()を呼び出し、受け取った島名を返す

改善したコード

結論から書くと、指摘を受けて、クラスの継承を導入することにしました。

改善前のコードの問題は、IslandクラスとHintクラスの役割分担ができていないことでした。継承を使って、ヒントのポリモーフィズムを実現することで、改善できると考えました。

作成したHintのサブクラスとIslandクラスの抜粋を掲載します。

Hintのサブクラス

<?php
class OddHint extends Hint
{
    public function isMatched($condition, int $numberOfDice, int $goldTotal) : bool
    {
        if ($condition === parent::ODD) {
            if ($numberOfDice % 2 === 1 && $goldTotal >= $this->requiredGold) {
                return true;
            }
        }

        return false;
    }
}

Islandクラス

<?php

class Island
{
    // ... 

    public function getNext(int $numberOfDice, int $goldTotal)
    {
        foreach ($this->hintCollection as $condition => $hint) {
            if ($hint->isMatched($condition, $numberOfDice, $goldTotal)) {
                return $hint->getNext();
            }
        }

        throw new \Exception('当てはまるヒントがありません');
    }
    
}

ヒントに関する処理を、Hintのサブクラスへカプセル化しました。改善後は、IslandクラスとHintクラスの役割分担がはっきりしました。そして、IslandクラスのgetNext()は、Hintのサブクラスが提供するisMatched()とgetNext()を使うだけの処理になり、ヒントの詳細を知る必要がなくなりました。

クラスの役割まとめ

改善点を整理するために、改善後のクラスの役割をまとめます。

Hintクラス

  • ゴールに必要なゴールド、進む島名、獲得ゴールドをプロパティとして持つ
  • ヒントの条件を満たしているかを判定するisMatched()メソッドを提供する。例えば、OddHintクラスのisMatched()メソッドは、コンディションが奇数(odd)で出目の和と総ゴールドの条件を満たすとき、trueを返す
  • getNext()は保持している進む島名、獲得ゴールドを返す

Islandクラス

  • ヒントの条件（「数字」・「奇数」・「偶数」・「その他」）に応じて、Hintクラスのサブクラスのインスタンスを作成する。例えばヒントが奇数のときはOddHintクラスのインスタンスを作成する
  • 自身が保持しているhintインスタンスのisMatched()へコンディション、出目の和、総獲得ゴールドを渡し、trueを受け取ったときは、同インスタンスのgetNext()を呼び出す

クラス図/シーケンス図

参考までに、変更前と変更後の、簡単なクラス図とシーケンス図を掲載します。

クラス図

変更前 変更後
class1 class-after

シーケンス図

変更前 変更後
sequence-before sequence-after

参考文献

さらなる改善

今回、私が考えた変更は、サブクラスを使い、ポリモーフィズムを実現するところまででした。その後、教育担当者から別の解き方のコードが示されました。以下に、そのコードを見て、考えた内容を書きます。

示されたコードを掲載します。

Hintクラス

<?php
class Hint
{
    private $f;

    public function __construct(Callable $f)
    {
        $this->f = $f;
    }

    public function evaluate(DiceInterface $dice, PlayerStatus $playerStatus)
    {
        $refl = new \ReflectionFunction($this->f);
        $parameters = $refl->getParameters();

        $args = [];
        try {
            foreach ($parameters as $paramInfo) {
                /** @var \ReflectionParameter $paramInfo */
                switch ($paramInfo->getName()) {
                    case 'a':
                    case 'b':
                        $args[] = $dice->get();
                        break;
                    case 'playerStatus':
                        $args[] = $playerStatus;
                        break;
                }
            }

            return call_user_func_array($this->f, $args);
        } catch (\Exception $e) {
            return new Stop();
        }
    }
}

Hintクラス生成箇所

<?php
$builder = new GameBuilder();
$builder->addIsland(
    new Island('S', new Hint(function($a) {
        switch ($a) {
            case 1:
                return new Hint\Result('A');
            case 2:
                return new Hint\Result('B');
            case 6:
                return new Hint\Result('C');
        }

        return new Hint\NopResult();
})))
->addIsland(
    new Island('A', new Hint(function($a) {
    switch ($a) {
        case 3:
            return new Hint\Result('B');
        case 4:
            return new Hint\Result('C');
    }

    return new Hint\Result('S', 100);
})))
// ...
->addIsland(
    new Island('C', new Hint(function($a, $b) {
    switch (($a + $b) % 2) {
        case 0:
            return new Hint\Result('E', 100);
        case 1:
            return new Hint\Result('D', 200);
    }

    return new Hint\NopResult();
})))
// ...

Islandクラスのコンストラクタの引数で、コールバック関数を渡しています。この設計だと、インスタンスを作成するときに、データに対する処理内容を注入することができます。ヒントをコールバック関数で渡しているので、新しい種類のヒントを追加するときでも、コールバック関数を変更することで対応できます。

例えば、ランダムに決まるラッキナンバーの値が7のときはゴールという処理も可能な柔軟性の高いコードになっています。

<?php
if (rand(1, 10) === 7) {
    return new Hint\Result('G');
}

まとめ

今回の改善では、継承を使い、ポリモーフィズムを実現することで、クラスの役割を整理しました。さらにコールバック関数を使い、問題をとても柔軟に解決する方法を知りました。

最初はコード設計が上手くできず、とにかく動作するコードを書き、レビューで指摘され、修正をする毎日でした。しかし、レビューを受けて自分のコードが改善されていく経験を通して、コード設計の大切さを、言葉だけでなく、実感として感じ始めています。今後は、新人研修で得た知識を自分のコード設計に落とし込んでいきたいと考えています。

最後に

教育担当の後藤の記事「プログラマーの設計力を伸ばすための土台づくり」にもあるように、新人研修の中で期待されている成果（学び）をしっかり意識して、できるところから挑戦しています。とても恵まれた環境で新人研修を受けているので、悪戦苦闘しながらも、何としても成長したいという思いを胸に、新人研修に取り組んでいます。

新人研修で教育担当者からレビューを受けることで、考えていたよりもとても多くの学びを得ています。他の人にコードをレビューされることに抵抗がある方（私も最初は抵抗がありました）も、レビューを受けることに挑戦することで、多くの学びを得られると思います。私も、引き続きレビューを受けることで多くの学びを得ていきたいと思います。