仕様変更に耐えるように"今の"DRY原則を考える

こんにちは〜！
NE株式会社のはやしまき（@_mkmk884）です🦒

DRY（Don't Repeat Yourself）原則はコードの重複を減らし、保守性を高める効果的な手法ですが、適用の仕方によっては仕様変更に対応できなくなることがあります。
特に弊社が開発しているネクストエンジンは多くの他サービスとも連携しているため、ネクストエンジン内の仕様変更だけでなく、外部連携サービスの仕様変更もあります。

当時の仕様的にはDRY原則に沿っていたものが、時間とともに保守性を損ない、結果的にDRYではなくなり仕様変更に耐えなくなったケースについて、今回は書いていこうと思います！

DRY原則とは

DRY原則（Don't repeat yourself）とは、ソフトウェアの構成や構築手法についての原則の一つで、同じ意味や機能を持つ情報を複数の場所に重複して置くことをなるべく避けるべきとする考え方です。

複数の場所に同じことを書くと、変更が生じたときに複数の同じ処理に対して一気に変更を加えなければならないため、一箇所で管理しようという原則です。

今回の問題

今回私が直面した問題は、二つの似た処理を一つのメソッドに統合し、フラグで細かい違いを切り替えるコードに対して、片方の処理に変更を加える際にもう片方に影響がでるという問題でした。
問題点が起こった処理は、外部連携サービスをもとに作成された情報（以降、サービスデータを表します）を削除する処理の部分でした。
サービスデータはネクストエンジンの形式で作成された情報（以降、NEデータと表します）に紐づいています。

NEデータとサービスデータの関係性

このサービスデータを削除する処理は2つの画面から呼ばれます。

• A画面：NEデータの一覧画面（削除したいサービスを選択することでNEデータに紐づくサービスデータを削除することができる）
• B画面：サービスデータの一覧画面

対象の削除メソッド（不要なところは簡略化しています）

public function delete(array $service_codes, boolean $is_from_syohin, ...): array
{
    ...

    if ($is_from_A) {
        // A画面からのみ、必要な配列を作る
    }

    foreach ($service_codes as $service_code) {
        ...

        // 削除対象のレコードIDを取得する
        // ユニークコードがキーで削除対象レコードIDが値
        if ($is_from_A) { // A画面からの処理
            ...
            // 以下のような配列が出来上がる（レコードがないものは、nullとして入れる）
            $delete_ids = [
                'data_code01' => 1,
                'data_code02' => null,
                'data_code03' => 3,
            ];
        } else { // B画面からの処理
            // 以下のような配列が出来上がる（レコードが存在するものしかないため、値がnullになることはない）
            $delete_ids = [
                'service_code01' => 1,
                'service_code123' => 2,
            ];
        }

        // 削除する処理
        ...

        // 結果画面の表を出すための処理
        ...
    }
    return $results;
}

削除対象のサービスデータのIDを取得し、ユニークコードをキーとして削除対象のレコードのIDを持つ配列を作り、そのIDでまとめて削除するということを削除メソッドの中身でやっていました。
サービスデータのユニークコードを持つカラム名はNEデータを扱うクラス、サービスデータを扱う各サービスのクラスのそれぞれのプロパティに定義していました。
また、2つの画面のうちどちらから呼ばれているかを管理するためにフラグ（以降、$is_from_Aとします）を用いていました。

今回の仕様変更箇所

そこで今回、複数のサービスのうちの一つに仕様の変更がありました。私たちがB画面にてユニークコードとして扱いたいコードがユニークではなくなり、2つの値を組み合わせることでユニークになるようになったのです。
もともと、この削除メソッドが書かれている箇所はFactoryパターンを用いたガチガチの継承クラスになっており、2つの値をユニークコードとしてキーにするのは難しくなりました😢
今回、B画面からの削除処理で作られる配列では、削除対象のレコードのIDをキーにすることにしました。

配列の形式変更前

$delete_ids = [
    'service_code01' => 1,
    'service_code123' => 2,
];

配列の形式変更後

$delete_ids = [
    1 => 'service_code01',
    2 => 'service_code123',
];

しかし、A画面から削除する場合は、紐づくサービスのレコードがない可能性があるため、ユニークコードに紐づくレコードのIDはnullとして配列を作らなければならなく、ユニークコードをキーとしたままにする必要がありました。

このように、配列の形式等細かい仕様が変わってきたことにより、片方の仕様変更によりもう片方が引っ張られる状況になりました😵

今考えるこれからも変わらない部分

ここで、当時考慮していたであろう変わらない部分と変わる部分を考えてみました。

• 変わらない部分（2つの画面で共通なこと）
  • 処理中で扱う配列が、1種類の値で構成されたユニークコードがキーで値が削除対象のレコードであること
  • IN/OUTの値の構成が同じであること
  • サービスデータを消すこと
• 変わる部分
  • 各サービスの仕様
    • 各サービスのユニークコードに値する値（どのカラムの値か）

私の洗い出した上記の当時の考慮内容と今回の課題をあわせて考えたとき、他サービスが関わる今回の機能の「変わらない部分」とは「自分たちのサービス内で決めれること」と考えられると思いました。

• 変わらない部分（ネクストエンジンの仕様とするもの）
  • サービスデータを消すこと
• 変わる部分
  • 各サービスの仕様
    • 各サービスのデータの構成方法
  • 上記に伴う処理中で扱う配列
  • IN/OUTの値の構成

どう変えていったか

B画面の仕様変更の追従の前に、2画面同士の依存を剥がすリファクタリングを行いました。
具体的には以下のような手順で行いました。

1. A画面からの削除処理とB検索結果画面からの削除処理のメソッドを分け、$is_from_A_pageフラグをもとに呼び出すメソッドを切り替える
   • 中身の処理は変えず、メソッドを複製してそれぞれに新しいメソッド名を命名する
   • テストコードも分ける
2. AとBのそれぞれの引数とメソッド内の$is_from_Aフラグを消す
   • A画面用のメソッドからは$is_from_Aフラグの分岐内の処理を残す
   • B画面用のメソッドからは$is_from_Aフラグの分岐外の処理を残す
3. 仕様変更の追従をB画面用のメソッドで行う

今更こんな当たり前なリファクタリングの手順……と思うかもしれませんが、結構前に書かれているようなコードだと、自分が認識していない考慮点が思わず出てきたりします。

そこで、それぞれの手順の1つずつでテストを回すことが重要です🔁
不具合が起こらないかどうか、考慮すべき点が漏れていないかどうかを確かめる必要があります。
また、そのためにも普段から価値のあるテストコードを書いておいたり、テストコードが心もとないときは、ちゃんとテストを書いてからリファクタリングに移るようにしましょう！

学び

変わらない部分と変わる部分を明確にする

今回のケースの改修を行ったことにより、私は今の段階のものでいいので、変わらない部分と変わる部分を明確にしておくことが大切だと感じました。
正直、将来どういう変化が起こるかは可能性でしかないと思います。今回私は「変わらない部分」とは「自分たちのサービス内で決めれること」としましたが、自分たちのサービス内でも仕様変更はあります。
できるだけ耐えられるように"今"のものを"今"考えておく、そしてそれをコードに落とし込んでおくが、今後の仕様変更に比較的耐えられるのではと思いました💡

また、できるだけ決定を遅らせるという手段もあります。変更が容易に加わりそうなところほど、あえて共通化をせず、ある程度固まってから合わせるというのもあると実感しました。

上記の発表が私の記憶に残っています💭
データモデルの話ではありますが、ソースコードの設計においても同じようなことが言えるのではないかなと私は思いました。（特にスライド18ページ目）

ユーザーフローが異なる場合はメソッドも分ける

今回、DRY原則を取り上げて記事を書きましたが、異なるユーザーフローの処理が同じメソッドを通っているのはSOLID原則の中の「単一責任原則」に違反しています。

「クラスが担う責任は、たったひとつに限定すべき」とする設計原則が単一責任原則です。
https://qiita.com/MinoDriven/items/76307b1b066467cbfd6a

「今考えるこれからも変わらない部分」の章で記載した通り、
今回の改修前のメソッドにおいて、2画面からの処理で共通であったことは具体的な実装の部分のみです。

• 処理中で扱う配列が、1種類の値で構成されたユニークコードがキーで値が削除対象のレコードであること
• IN/OUTの値の構成が同じであること

具体的な実装はたまたま同じであっただけで、

• NEデータ一覧からNEデータに紐づくサービスデータを特定する
• サービスデータ一覧からサービスデータを特定する

と、それぞれ概念は異なります。また、今後も画面や機能としての仕様が根本から変わらない限り、不変です。

同じようなロジックでも概念や振る舞いが異なれば、別にするべきであると再度実感しました😤

おわりに

書籍等で実際に概念を学んでいた設計原則について、新しい開発の設計に対して意識して適用するというのは意識しているつもりではありますが、今回実際に仕様変更をしなければいけない状況で"変更しづらい"コードに直面したことで、実体験が学びになりました！

システムやサービスが稼働している以上、変更は切っても切り離せないものであり、今後も変更に耐える設計を"今"考えていくことで、自分含め将来変更を加える人がより楽かつバグなしで変更を加えれるようになり、ユーザーに価値を早く届けていけるようにしていきましょう〜💪