Help us understand the problem. What is going on with this article?

今日から始められるリファクタリング10選 (1)

More than 3 years have passed since last update.

概要

  • Martin Fowler 著「リファクタリング―プログラムの体質改善テクニック」を参考にしました
  • 手軽にできる (と思われる) リファクタリングを抜粋しました (10 という数字に特に意味はなくキリがよかったので)
  • リファクタリングカタログ名は「」で括り、()内にページ番号を振りました
  • サンプルコードはPHPで記述しました
  • テストコードがある前提です

「リファクタリング―プログラムの体質改善テクニック」について

この本はリファクタリングのガイドブックです。系統だった効果的なリファクタリング手法を説明しています。コード中にバグを加えずに、ソフトウェアの構造を体系的に改善できます。

(Amazon 商品の説明より)

オリジナル版と新装版があります。本稿ではオリジナル版を使用しました (新装版と目視で比較したが、リファクタリングカタログに違いはないみたいだった)。

リファクタリング―プログラムの体質改善テクニック

新装版 リファクタリング―既存のコードを安全に改善する

手軽にできるリファクタリングとは

短時間 (自分のコーディングのスピードを基準にするなら2時間以内くらいか) で終わるような、比較的簡単なものを選んであります。
具体的には、「クラスの抽出 (149)」のようにモデリングに関わるものや「Template Methodの形成 (345)」のように複数クラスの複数メソッドにまたがる修正でなく、単一のクラスに関わるもの、メソッド内の処理の一部分 (条件文ブロック) に関わるもの、が対象です。
小さなリファクタリングであっても、後の大きなリファクタリングの助けになることが多いと思うので、大きなリファクタリングの布石として行うといいと思います。

サンプルコードについて

実際に過去に関わったプロジェクトのコードをアレンジして使用しています。

手軽にできるリファクタリングカタログ10選

  1. メソッドの抽出 (110)
  2. 一時変数のインライン化 (119)
  3. アルゴリズムの取り替え (139)
  4. 条件記述の分解 (238)
  5. 条件記述の結合 (240)
  6. 重複した条件記述の断片の統合 (243)
  7. ガード節による入れ子条件記述の置き換え (250)
  8. 引数の追加 (275)
  9. 問い合せと更新の分離 (277)
  10. 例外によるエラーコードの置き換え (310)

第1回 (本稿) は、第6章「メソッドの構成」に属する 1〜3 について。
第2回では、第9章「条件記述の単純化」に属する 4〜7 について。
第3回では、第10章「メソッド呼び出しの単純化」に属する 8〜10 について。

1. メソッドの抽出 (110)

概要

コードの断片をメソッドにして、それに目的を表すような名前をつける。

サンプルコード(リファクタリング前)

よくある、CSVファイルの中身を読んできて配列に入れ、それぞれの行に対して何かしらの処理を行ってデータベースを更新する、といった処理。
データベースの更新をループの外に出している点はいいと思うけど、いかんせんループ内の処理が長くて、可読性が低くなってしまっています。

Importer.php
public function importCsv($file)
{
    $data = $this->readCsv($file);
    foreach ($data as $i => $row) {
        $year = $row[0];
        $month = $row[1];
        $userId = $row[2];

        $user = User::find($userId);
        if (!$user) {
            $this->appendError('User not found.');
            continue;
        }

        // 長い処理が続く

        $importedData[] = $this->getImportedData($year, $month, $user->getId());
    }
    $this->updateModel($importedData);
}

手順

  1. 新しくメソッドをつくってループ内のコードをごそっと移動。
  2. continue していたところは return に置き換える。

サンプルコード(リファクタリング後)

ループ内の処理をメソッドに切り出すと、元のメソッド (この例では importCsv が何をやっているのかが分かりやすくなるし、見た目もすっきりしていいと思います。

Importer.php
public function importCsv($file)
{
    $data = $this->readCsv($file);
    foreach ($data as $i => $row) {
        $importedData = $this->getImportedDataWithArray($row);
        if (empty($importedData)) {
            continue;
        }
        $importedDataList[] = $importedData;
    }
    $this->updateModel($importedDataList);
}

private function getImportedDataWithArray($row)
{
    $year = $row[0];
    $month = $row[1];
    $userId = $row[2];

    $user = User::find($userId);
    if (!$user) {
        $this->appendError('User not found.');
        return false;
    }

    // 長い処理が続く...

    return $this->getImportedData($year, $month, $user->getId());
}

2. 一時変数のインライン化 (119)

概要

一時変数への参照をすべて式で置き換える。
ほとんどの場合、「問い合せによる一時変数の置き換え (120)」と組み合せて使用する。

サンプルコード(リファクタリング前)

本には「他のリファクタリングの邪魔になったときに、インライン化すればいい」と書いてあるが、ちょっとどういうケースを想定しているのか分かりませんでした。
それは置いといて、以下のようなケースが本リファクタリングを適用するケースに該当するんじゃないかと思います。
yyyy-mm-dd 形式の日付を年・月・日の配列に変換する処理がこんな風に書いてあったので、リファクタリングしてみます。

Exporter.php
public function exportUsersToCsv($file)
{
    $users = User::findAll();
    $rows = [];
    foreach ($users as $user) {
        $row['id'] = $user->getId();
        $row['email'] = $user->getEmail();
        $birthdayArray = empty($user->getBirthday()) ? ['', '', ''] : explode('-', $user->getBirthday());
        $row['birthdayYear'] = $birthdayArray[0];
        $row['birthdayMonth'] = $birthdayArray[1];
        $row['birthdayDay'] = $birthdayArray[2];

        // 続く

        $rows[] = $row;
    }

    $this->writeToCsv($file, $rows);
}

手順

  1. 問い合せによる一時変数の置き換えを行う。
  2. 一度しか使われていない一時変数をメソッドに置き換える。

サンプルコード(リファクタリング後)

リファクタリング後は、$birthdayArray が消えました。

Exporter.php
public function exportUsersToCsv($file)
{
    $users = User::findAll();
    $rows = [];
    foreach ($users as $user) {
        $row['id'] = $user->getId();
        $row['email'] = $user->getEmail();
        list($row['birthdayYear'], $row['birthdayMonth'], $row['birthdayDay']) = $user->getBirthdayAsArray();

        // 続く

        $rows[] = $row;
    }

    $this->writeToCsv($file, $rows);
}
User.php
public function getBirthdayAsArray()
{
    return empty($this->getBirthday()) ? ['', '', ''] : explode('-', $this->getBirthday());
}

日付を文字列から配列に変換する処理は汎用性があるので、もう一歩進めて、ConvertStringDateToArray などという名前のメソッドをどっかにつくってもいいんじゃないかと思います。

Date.php
public static function ConvertStringDateToArray($dateString)
{
    return empty($dateString) ? ['', '', ''] : explode('-', $dateString);
}
Exporter.php
public function exportUsersToCsv($file)
{
    //略
        list($row['birthdayYear'], $row['birthdayMonth'], $row['birthdayDay']) = Date::ConvertStringDateToArray($user->getBirthday());
    //略
}

3. アルゴリズムの取り替え (139)

概要

メソッドの本体を新たなアルゴリズムで置き換える。

実装したときはそれが最良だと思っても後から見直すと何じゃこりゃって思うのはしょっちゅうあるし、だれだこんなコード書いたのつってlog を見ると自分だったりするので、そういうコードに遭遇したときはさくっとこのリファクタリングを行うのがいいと思います。

サンプルコード(リファクタリング前)

下記のコードは、Doctrine DBAL を使ってSQL文を構築する箇所からの抜粋。

これを書いたひとはおそらく =LIKE が混在している検索条件を見て、これをメソッド化するのは大変だと考えたんでしょう。
でも、各条件文で異なる箇所を抜粋して構造化すれば、今後パラメーターが増えてもコピペする必要のないアルゴリズムにすることは可能です。

UserRepository.php
public function findUser($condition)
{
    $queryBuilder = $this->conn->createQueryBuilder();

    // 略

    if (isset($condition['userId']) && trim($condition['userId']) !== '') {
        $queryBuilder->andWhere('u.id = :userId');
        $param = array_merge($param, [':userId' => $condition['userId']]);
    }
    if (isset($condition['email']) && trim($condition['email']) !== '') {
        $queryBuilder->andWhere('u.email LIKE :email');
        $param = array_merge($param, [':email' => $condition['email']]);
    }

    // 長く続く

    $stmt = $this->conn->prepare($queryBuilder);
    $stmt->execute($param);

    return $stmt->fetchAll();
}

手順

  1. 置き換えたい箇所を新しいアルゴリズムで置き換える。

サンプルコード(リファクタリング後)

各条件のパラメーターを $options として配列に押し込んで、buildParameters (メソッドの抽出だ!) に渡すことで、長い if 文の連なりを排除することができました。

細かい点ですが、if 文の条件を反転させ、「ガード節による入れ子条件記述の置き換え (250)」を適用してあります。

UserRepository.php
public function findUser($condition)
{
    $queryBuilder = $this->conn->createQueryBuilder();

    // 略

    $options = [
        'userId' => ['field' => 'u.id', 'operator' => '='],
        'email'  => ['field' -> 'u.email', 'operator' => 'LIKE'],
    ];

    $params = $this->buildParameters($condition, $queryBuilder, $options);

    $stmt = $this->conn->prepare($queryBuilder);
    $stmt->execute($params);

    return $stmt->fetchAll();
}

private function buildParameters($condition, $queryBuilder, $options)
{
    $params = [];
    foreach ($options as $key => $option) {
        $value = trim($condition[$key]);
        if (!isset($condition[$key]) || $value === '') {
            continue;
        }
        $sql = sprintf('%s %s :%s', $option['field'], $option['operator'], $key);
        $queryBuilder->andWhere($sql);
        $params[':' . $key] = $value;
    }

    return $params;
}

いつになるか分かりませんが以下次号。

書きました、今日から始められるリファクタリング10選 (2)

Why not register and get more from Qiita?
  1. We will deliver articles that match you
    By following users and tags, you can catch up information on technical fields that you are interested in as a whole
  2. you can read useful information later efficiently
    By "stocking" the articles you like, you can search right away
Comments
No comments
Sign up for free and join this conversation.
If you already have a Qiita account
Why do not you register as a user and use Qiita more conveniently?
You need to log in to use this function. Qiita can be used more conveniently after logging in.
You seem to be reading articles frequently this month. Qiita can be used more conveniently after logging in.
  1. We will deliver articles that match you
    By following users and tags, you can catch up information on technical fields that you are interested in as a whole
  2. you can read useful information later efficiently
    By "stocking" the articles you like, you can search right away
ユーザーは見つかりませんでした