株式会社ソニックムーブPublicationへの投稿
🏥

新卒1年目の頃に書いたコードを読み直したら汚すぎて体調が悪くなった話

MatsuzakiSMV
に公開
Laravel
PHP
リファクタリング
リーダブルコード
テスト駆動
tech

はじめに

汚すぎて読んでいると体調が悪くなるコードってありますよね。
僕が新卒1年目の頃に書いたコードを読み返していたらそんなコードを見つけてしまいました。
それを以下に添付します。
※言語はPHP、フレームワークはLaravelを使っています
※眺めるだけで大丈夫です

/**
 * 実行メソッド
 *
 * @param  IndexLatestReservationsInputInterface  $input  入力データ
 * @return IndexLatestReservationsOutput
 */
public function execute(IndexLatestReservationsInputInterface $input): IndexLatestReservationsOutput
{
    $userId = $input->getUserId();

    // 今日以降のセミナーアンケートとそれに紐づく予約、ルームのDTOを全て取得
    $seminarQuestionnaireLatestReservationsRoomDTOs = $this->seminarQuestionnaireRepository
        ->getSeminarQuestionnaireLatestReservationsRoomDTOs(
            $userId,
            (int) Carbon::now()->format('Ymd'),
        );

    // 必要な情報だけの配列に整形
    $latestReservationsArray = [];
    foreach ($seminarQuestionnaireLatestReservationsRoomDTOs as $seminarQuestionnaireLatestReservationsRoomDTO) {
        $seminarQuestionnaire = $seminarQuestionnaireLatestReservationsRoomDTO->getSeminarQuestionnaire();
        $seminarType = $seminarQuestionnaire->getSeminarType();

        // 対面かオンラインかで分岐する箇所
        if ($seminarType === Enum::getValue('common.seminarType.seminarType_2')
            || $seminarType === Enum::getValue('common.seminarType.seminarType_4')
        ) {
            // セミナータイプがオンラインの場合はルーム名を「オンライン」とする
            $roomName = Room::ONLINE_ROOM_NAME;

            // ユーザーアンケートを取得
            $userQuestionnaire = $this->userQuestionnaireRepository->findByUserId($userId);
            if (is_null($userQuestionnaire)) {
                throw new UserQuestionnaireNotFoundException();
            }
            $meetingUrl = $userQuestionnaire->getMeetingUrl();
        } else {
            // セミナータイプが対面の場合はルーム名を取得する
            $room = $seminarQuestionnaireLatestReservationsRoomDTO->getRoom();
            $roomName = $room->getName();
            $meetingUrl = null;
        }

        // その日の予約コマの配列作成
        $seminars = [];
        $reservations = $seminarQuestionnaireLatestReservationsRoomDTO->getReservations();
        foreach ($reservations as $reservation) {
            /** @var ReservationEntity $reservation */
            // 今がキャンセル可能かどうかを取得
            $isCancelable = $this->getIsCancelableService->execute($reservation);

            $seminars[] = [
                'startTime'    => $reservation->getStartTime(),
                'isCancelable' => $isCancelable,
            ];
        }

        // セミナータイプによって分岐する箇所
        if ($seminarType === Enum::getValue('common.seminarType.seminarType_1')
            || $seminarType === Enum::getValue('common.seminarType.seminarType_2')
        ) {
            // セミナータイプが通常の時はセミナー事前アンケートが提出されてるかどうかを取得
            $isSubmitted = $this->seminarQuestionnaireRepository->hasSeminarQuestionnairePreparation($seminarQuestionnaire->getId());
        } else {
            // セミナータイプがトライアルの時はトライアル用アンケートが提出されてるかどうかを取得
            $trialQuestionnaire = $this->trialQuestionnaireRepository->findByUserId($userId);
            if (is_null($trialQuestionnaire)) {
                throw new TrialQuestionnaireNotFoundException();
            }
            $isSubmitted = $trialQuestionnaire->getIsSubmitted();
        }

        $latestReservationsArray[] = [
            'seminarStartDate' => $seminarQuestionnaire->getSeminarDateText(),
            'seminarType'      => $seminarType,
            'roomName'         => $roomName,
            'seminars'         => $seminars,
            'isSubmitted'      => $isSubmitted,
            'meetingUrl'       => $meetingUrl,
        ];
    }

    return new IndexLatestReservationsOutput($latestReservationsArray);
}

正直とても読む気が起こらないですよね。
ちなみにこれは「直近のセミナー予約データを配列にして返す」ユースケースのメソッドです。

今回はこのコードを自戒と成長のためにリファクタリングしてみました。
参考にした本は「リーダブルコード」です。

この本を読み、コードを綺麗に書くための様々な手法を学ぶことができました。
そこで、それらの手法を取り入れたリファクタリングのフローを考えて実践してみたので、今回はそのフローに沿ってリファクタリングしていきます。

リファクタリングのフロー

各ステップではテストを実行し、動作を確認しながら進めます。

  1. タスクの列挙
  2. タスクを分割
  3. 変数の追加や削除
  4. 命名の修正
  5. コメントの追加や削除

家の内装を整えてから外装を綺麗にするようなイメージです。

補足

  • このメソッドのテストコードは既に書けています
  • テストコードのリファクタリングはしないです

テストコードが既に書けていることを活かして、「2. タスクをメソッドに分割」を行なった後、テストを実行しエラーになることを確認し、その後は常にテストが成功するように保ちながら修正していくことで、安心してリファクタリングできると考えました。
このフローは「リーダブルコード」で紹介されていた手法とテスト駆動開発のフローを組み合わせるような形で考えました。
※テスト駆動開発とは何かについてはここでは説明をしません。

リファクタリング実践

1. タスクの列挙

まず、このメソッドが行っているタスクを以下のように列挙しました。

  • 今日以降のセミナーアンケートとそれに紐づく予約、ルームのDTOを全て取得
  • ルーム名を取得
  • オンラインミーティングUrl取得
  • セミナー開始時間とキャンセル可能フラグを取得
  • アンケート提出状況フラグを取得
  • 配列にしてアウトプットする

2. タスクを分割

2-1 タスクをメソッドに分割

列挙したタスクを見ながら、「ここがこうなったら読みやすいだろうなぁ」という願望を込めて、タスクをメソッドに分割します。

コードを眺めていて思いましたが、このコードが読みづらい原因は間違いなく foreach の中のネストが深いから です。

列挙したタスクのうち、foreach の中で行われているタスクをまずメソッドに分割してみます。

foreach ($seminarQuestionnaireLatestReservationsRoomDTOs as $seminarQuestionnaireLatestReservationsRoomDTO) {
    // ...

    // ルーム名を取得
    $roomName = $this->getRoomName();

    // オンラインミーティングUrl取得
    $meetingUrl = $this->getMeetingUrl();
    
    // セミナー開始時間とキャンセル可能フラグを取得
    $seminars = $this->getSeminars();

    // アンケート提出状況フラグを取得
    $isSubmitted = $this->getIsSubmitted();

    // ...
}

この段階でテストを実行し、エラーになることを確認します。

2-2 テストが通るように最低限の修正

メソッドに必要な引数を渡して、テストが通るように修正します。

// ルーム名を取得
$roomName = $this->getRoomName(
    seminarType: $seminarType,
    seminarQuestionnaireLatestReservationsRoomDTO: $seminarQuestionnaireLatestReservationsRoomDTO,
);

// オンラインミーティングUrl取得
$meetingUrl = $this->getMeetingUrl(
    seminarType: $seminarType,
    userId: $userId,
);

// セミナー開始時間とキャンセル可能フラグを取得
$seminars = $this->getSeminars(
    seminarQuestionnaireLatestReservationsRoomDTO: $seminarQuestionnaireLatestReservationsRoomDTO,
);

// アンケート提出状況フラグを取得
$isSubmitted = $this->getIsSubmitted(
    seminarType: $seminarType,
    seminarQuestionnaire: $seminarQuestionnaire,
    userId: $userId,
);

各ヘルパメソッドの実装は以下のようになりました。

getRoomName

private function getRoomName(
    int $seminarType,
    SeminarQuestionnaireLatestReservationsRoomDTO $seminarQuestionnaireLatestReservationsRoomDTO,
): string {
    if ($seminarType === Enum::getValue('common.seminarType.seminarType_2')
        || $seminarType === Enum::getValue('common.seminarType.seminarType_4')
    ) {
        // セミナータイプがオンラインの場合はルーム名を「オンライン」とする
        $roomName = Room::ONLINE_ROOM_NAME;
    } else {
        // セミナータイプが対面の場合はルーム名を取得する
        $room = $seminarQuestionnaireLatestReservationsRoomDTO->getRoom();
        $roomName = $room->getName();
    }

    return $roomName;
}

getMeetingUrl

private function getMeetingUrl(
    int $seminarType,
    int $userId,
): ?string {
    // 対面かオンラインかで分岐する箇所
    if ($seminarType === Enum::getValue('common.seminarType.seminarType_2')
        || $seminarType === Enum::getValue('common.seminarType.seminarType_4')
    ) {
        $userQuestionnaire = $this->userQuestionnaireRepository->findByUserId($userId);
        if (is_null($userQuestionnaire)) {
            throw new UserQuestionnaireNotFoundException();
        }
        $meetingUrl = $userQuestionnaire->getMeetingUrl();
    } else {
        $meetingUrl = null;
    }

    return $meetingUrl;
}

getSeminars

private function getSeminars(
    SeminarQuestionnaireLatestReservationsRoomDTO $seminarQuestionnaireLatestReservationsRoomDTO,
): array {
    // その日の予約コマの配列作成
    $seminars = [];
    $reservations = $seminarQuestionnaireLatestReservationsRoomDTO->getReservations();
    foreach ($reservations as $reservation) {
        /** @var ReservationEntity $reservation */
        // 今がキャンセル可能かどうかを取得
        $isCancelable = $this->getIsCancelableService->execute($reservation);

        $seminars[] = [
            'startTime'    => $reservation->getStartTime(),
            'isCancelable' => $isCancelable,
        ];
    }

    return $seminars;
}

getIsSubmitted

private function getIsSubmitted(
    int $seminarType,
    SeminarQuestionnaireEntity $seminarQuestionnaire,
    int $userId,
): bool {
    // セミナーかトライアルかで分岐する箇所
    if ($seminarType === Enum::getValue('common.seminarType.seminarType_1')
        || $seminarType === Enum::getValue('common.seminarType.seminarType_2')
    ) {
        // セミナータイプが通常の時はセミナー事前アンケートが提出されてるかどうかを取得
        $isSubmitted = $this->seminarQuestionnaireRepository->hasSeminarQuestionnairePreparation(
            $seminarQuestionnaire->getId()
        );
    } else {
        // セミナータイプがトライアルの時はトライアル用アンケートが提出されてるかどうかを取得
        $trialQuestionnaire = $this->trialQuestionnaireRepository->findByUserId($userId);
        if (is_null($trialQuestionnaire)) {
            throw new TrialQuestionnaireNotFoundException();
        }
        $isSubmitted = $trialQuestionnaire->getIsSubmitted();
    }

    return $isSubmitted;
}

各ヘルパメソッドの中はまだ汚いですが、ひとまず、テストが成功するようになりました。

この後は変更を加えるたびにテストが通ることを確認しながら進めます。

3. 変数の修正

3-1. 「この変数は本当に必要か？」を考える

まず、 「この変数は本当に必要か？」という観点でリファクタリングします。

getRoomNameについて見てみます。
Before:

private function getRoomName(
    int $seminarType,
    SeminarQuestionnaireLatestReservationsRoomDTO $seminarQuestionnaireLatestReservationsRoomDTO,
): string {
    if ($seminarType === Enum::getValue('common.seminarType.seminarType_2')
        || $seminarType === Enum::getValue('common.seminarType.seminarType_4')
    ) {
        // セミナータイプがオンラインの場合はルーム名を「オンライン」とする
        $roomName = Room::ONLINE_ROOM_NAME;
    } else {
        // セミナータイプが対面の場合はルーム名を取得する
        $room = $seminarQuestionnaireLatestReservationsRoomDTO->getRoom();
        $roomName = $room->getName();
    }

    return $roomName;
}

After:

private function getRoomName(
    int $seminarType,
    SeminarQuestionnaireLatestReservationsRoomDTO $seminarQuestionnaireLatestReservationsRoomDTO,
): string {
    if ($seminarType === Enum::getValue('common.seminarType.seminarType_2')
        || $seminarType === Enum::getValue('common.seminarType.seminarType_4')
    ) {
        // セミナータイプがオンラインの場合はルーム名を「オンライン」とする
        return Room::ONLINE_ROOM_NAME;
    }
    // セミナータイプが対面の場合はルーム名を取得する
    return $seminarQuestionnaireLatestReservationsRoomDTO->getRoom()->getName();
}

このように早期リターンをすることで、$roomNameという不要な変数を削除できました。

メソッドに分割する前はそもそも早期リターンで変数を減らすという選択肢はなかったので、やはりメソッド分割の利点は大きいと感じます。

他のメソッドも同様に、早期リターンなどを使って変数の削除を行いました。（割愛）

3-2. 「変数を使うことで分かりやすくできないか？」を考える

今度は、変数で表していなかった箇所を逆に「変数を使うことで分かりやすくできないか?」という視点でリファクタリングします。

同じくgetRoomNameについて見てみます。
Before:

private function getRoomName(
    int $seminarType,
    SeminarQuestionnaireLatestReservationsRoomDTO $seminarQuestionnaireLatestReservationsRoomDTO,
): string {
    if ($seminarType === Enum::getValue('common.seminarType.seminarType_2')
        || $seminarType === Enum::getValue('common.seminarType.seminarType_4')
    )
    // ...
}

After:

private function getRoomName(
    int $seminarType,
    SeminarQuestionnaireLatestReservationsRoomDTO $seminarQuestionnaireLatestReservationsRoomDTO,
): string {
    $isOnlineSeminar = (
        $seminarType === Enum::getValue('common.seminarType.seminarType_2')
        || $seminarType === Enum::getValue('common.seminarType.seminarType_4')
    );

    if ($isOnlineSeminar) {
        // ...
    }
}

この $isOnlineSeminar のように、式の意味を説明するような変数のことを「リーダブルコード」では「説明変数」と呼ばれていました。
説明変数はメソッド内でタスクをさらに分割するような役割があります。

同様に他のメソッドも説明変数に入れて分かりやすくしました。（割愛）

その結果、getMeetingUrl でも同じくオンラインセミナーかどうかという観点で判定を行っているので $isOnlineSeminar という変数が発生しました。

つまり、別々のメソッド内で同じ役割の変数を生み出しているということは、その変数はメソッド内で生み出す必要がないということです。

そのため、$isOnlineSeminar はメソッド外部から引数で渡すようにしました。

private function getRoomName(
    bool $isOnlineSeminar,
    RoomEntity $roomEntity
): string {
    if ($isOnlineSeminar) {
        return Room::ONLINE_ROOM_NAME;
    }
    
    // ...
}

private function getMeetingUrl(
    bool $isOnlineSeminar,
    int $userId,
): ?string {
    if (!$isOnlineSeminar) {
        return null;
    }
    
    // ...
}

こうすることで重複する以下のコードを減らせました。

$isOnlineSeminar = (
        $seminarType === Enum::getValue('common.seminarType.seminarType_2')
        || $seminarType === Enum::getValue('common.seminarType.seminarType_4')
    );

4. 命名の修正

次に命名について考えます。

  • 「その変数名は具体的で明確か？」
  • 「その変数名が他の意味に誤解されることはないか？」

といった観点で見直します。

例えば、以下のヘルパメソッドを見てみます。

private function getSeminars(
    // ...
)

getSeminars という名前は少し抽象的です。

このメソッドは、コレクションを渡してその日の開始時刻を返すだけでなく、キャンセル可否の判定の役割も担っています。

それを踏まえて getSeminarAndCancelabilities という名前にし、誤解されないようにしました。

private function getSeminarAndCancelabilities(
    // ...
)

5. コメントの修正

最後にコメントについて考えます。

「コードからすぐにわかることをコメントに書かない」という考え方が大切です。

private function getRoomName(
    bool $isOnlineSeminar,
    RoomEntity $roomEntity
): string {
    if ($isOnlineSeminar) {
        // セミナータイプがオンラインの場合はルーム名を「オンライン」とする
        return Room::ONLINE_ROOM_NAME;
    }

    // セミナータイプが対面の場合はルーム名を取得する
    return $roomEntity->getName();
}

様々なリファクタリングを経てスッキリしたこのヘルパメソッドですが、2箇所あるコメントはどちらも「コードからすぐにわかること」なので不要です。

同様に他の不要なコメントを削除しました。（割愛）

リファクタリング後のコード

最終的に以下のようになりました。

/**
 * 実行メソッド
 *
 * @param  IndexLatestReservationsInputInterface  $input  入力データ
 * @return IndexLatestReservationsOutput
 */
public function execute(IndexLatestReservationsInputInterface $input): IndexLatestReservationsOutput
{
    $userId = $input->getUserId();

    // 今日以降のセミナーアンケートとそれに紐づく予約、ルームのDTOを全て取得
    $seminarQuestionnaireLatestReservationsRoomDTOs = $this->seminarQuestionnaireRepository
        ->getSeminarQuestionnaireLatestReservationsRoomDTOs(
            $userId,
            (int) Carbon::now()->format('Ymd'),
        );

    $latestReservationsArray = [];
    foreach ($seminarQuestionnaireLatestReservationsRoomDTOs as $seminarQuestionnaireLatestReservationsRoomDTO) {
        $seminarQuestionnaire = $seminarQuestionnaireLatestReservationsRoomDTO->getSeminarQuestionnaire();
        $seminarType = $seminarQuestionnaire->getSeminarType();

        $isOnlineSeminar = (
            $seminarType === Enum::getValue('common.seminarType.seminarType_2')
            || $seminarType === Enum::getValue('common.seminarType.seminarType_4')
        );

        $roomName = $this->getRoomName(
            isOnlineSeminar: $isOnlineSeminar,
            roomEntity: $seminarQuestionnaireLatestReservationsRoomDTO->getRoom(),
        );

        // セミナー開始時刻とキャンセル可否を配列で取得
        $seminarAndCancelabilities = $this->getSeminarAndCancelabilities(
            reservations: $seminarQuestionnaireLatestReservationsRoomDTO->getReservations(),
        );

        $isSubmitted = $this->getIsSubmitted(
            seminarType: $seminarType,
            seminarQuestionnaire: $seminarQuestionnaire,
            userId: $userId,
        );

        $meetingUrl = $this->getMeetingUrl(
            isOnlineSeminar: $isOnlineSeminar,
            userId: $userId,
        );

        $latestReservationsArray[] = [
            'seminarStartDate' => $seminarQuestionnaire->getSeminarDateText(),
            'seminarType'      => $seminarType,
            'roomName'         => $roomName,
            'seminars'         => $seminarAndCancelabilities,
            'isSubmitted'      => $isSubmitted,
            'meetingUrl'       => $meetingUrl,
        ];
    }

    return new IndexLatestReservationsOutput($latestReservationsArray);
}

/**
 * ルーム名を取得
 *
 * @param  bool        $isOnlineSeminar
 * @param  RoomEntity  $roomEntity
 * @return string
 */
private function getRoomName(
    bool $isOnlineSeminar,
    RoomEntity $roomEntity
): string {
    if ($isOnlineSeminar) {
        return Room::ONLINE_ROOM_NAME;
    }

    return $roomEntity->getName();
}

/**
 * オンラインミーティングUrlを取得
 *
 * @param  bool  $isOnlineSeminar
 * @param  int   $userId
 * @return string|null
 */
private function getMeetingUrl(
    bool $isOnlineSeminar,
    int $userId,
): ?string {
    if (!$isOnlineSeminar) {
        return null;
    }

    $userQuestionnaire = $this->userQuestionnaireRepository->findByUserId($userId);
    if (is_null($userQuestionnaire)) {
        throw new UserQuestionnaireNotFoundException();
    }

    return $userQuestionnaire->getMeetingUrl();
}

/**
 * 対象の予約日についてセミナー開始時間とキャンセル可能フラグを配列で返す
 *
 * @param  Collection<int, ReservationEntity>  $reservations
 * @return array<string, mixed>
 */
private function getSeminarAndCancelabilities(
    Collection $reservations,
): array {
    $seminars = [];
    foreach ($reservations as $reservation) {
        $isCancelable = $this->getIsCancelableService->execute($reservation);

        $seminars[] = [
            'startTime'    => $reservation->getStartTime(),
            'isCancelable' => $isCancelable,
        ];
    }

    return $seminars;
}

/**
 * アンケート提出状況フラグを取得
 *
 * @param  int                         $seminarType
 * @param  SeminarQuestionnaireEntity  $seminarQuestionnaire
 * @param  int                         $userId
 * @return bool
 */
private function getIsSubmitted(
    int $seminarType,
    SeminarQuestionnaireEntity $seminarQuestionnaire,
    int $userId,
): bool {
    $isSeminar = (
        $seminarType === Enum::getValue('common.seminarType.seminarType_1')
        || $seminarType === Enum::getValue('common.seminarType.seminarType_2')
    );

    if ($isSeminar) {
        return $this->seminarQuestionnaireRepository->hasSeminarQuestionnairePreparation(
            $seminarQuestionnaire->getId()
        );
    }

    $trialQuestionnaire = $this->trialQuestionnaireRepository->findByUserId($userId);
    if (is_null($trialQuestionnaire)) {
        throw new TrialQuestionnaireNotFoundException();
    }

    return $trialQuestionnaire->getIsSubmitted();
}

まとめ

今回、見るのも嫌だった自分のコードをフローを定めてリファクタリングしてみました。

結果として、とても良い感じに整理できたと感じています。

また、テスト駆動開発と組み合わせてリファクタリングすると、安心して修正できるため、テストコードの偉大さも再認識できました。

これからも汚いコードをみて体調を悪くすることがないよう綺麗にコードを書くことを意識し続けていこうと思います！

参考

Dustin Boswell, Trevor Foucher 著、角征典 訳『リーダブルコード ―より良いコードを書くためのシンプルで実践的なテクニック』オライリージャパン、2012年

株式会社ソニックムーブ
株式会社ソニックムーブPublication

BtoCサービスのUX・CRM設計とその実現を得意とする、デジタルコミュニケーションデザインに特化した開発会社です。ソニックムーブ所属エンジニアが情報発信していきます。

Discussion