【リファクタリング】問い合わせと更新の分離を丁寧にやってみた

はじめに

リファクタリング第 2 版の 11 章の「問い合わせと更新の分離」に
jest のテストを加えながら、一歩ずつリファクタリングを進めていきます.

• Before : 状態を変更しない冪等な「問い合わせ」と状態の変更を行う「更新」が混ざった関数
• After : 状態を変更しない冪等な「問い合わせ」関数と、状態の変更を行う「更新」関数

となるように関数を分けることをゴールとしてます

環境

v14.14.0 # node
26.6.3 # jest 

リファクタリング前

リファクタリング前は、alertForMiscreant は問い合わせと更新が見事に混ざった状態です。

index.js

import { setAlerms } from "./alerms";

// TODO : 問い合わせと更新の分離
export function alertForMiscreant(people) {
  for (const p of people) {
    if (p === "Don") {
      setAlerms();
      return "Don";
    }
    if (p === "John") {
      setAlerms();
      return "John";
    }
  }
  return "";
}

alerms.js

export function setAlerms() {
  // 何かしらの更新処理
}

リファクタリング手順

手順は以下のように進めていきます。

1. テストコードを書いて Green であることをテストする
2. 元の関数をコピーして新しい関数を作成する
3. 新しい関数をから、副作用の部分を取り除いて問い合わせ専用の関数とする
4. 元の関数の問い合わせ部分を、新しい関数で置き換える

1. テストコードの追加

setAlerms が適切に呼ばれているかをテストするコードを書いていきます。
冗長な初期化処理や、後処理はご容赦ください。

import { alertForMiscreant } from "../src/";
import * as alerms from "../src/alerms";

describe("問い合わせと更新の分離", () => {
  test("Don のとき setAlerms が 1 度呼ばれます", () => {
    const spy = jest.spyOn(alerms, "setAlerms").mockImplementation();
    const people = ["Don"];
    alertForMiscreant(people);
    expect(spy).toHaveBeenCalledTimes(1);
    spy.mockRestore();
  });

  test("John, Mar のとき setAlerms が 1 度呼ばれます", () => {
    const spy = jest.spyOn(alerms, "setAlerms").mockImplementation();
    const people = ["John", "Mar"];
    alertForMiscreant(people);
    expect(spy).toHaveBeenCalledTimes(1);
    spy.mockRestore();
  });

  test("Mar, Mar, Pon のとき setAlerms は度呼ばれません", () => {
    const spy = jest.spyOn(alerms, "setAlerms").mockImplementation();
    const people = ["Mar", "Mar", "Pon"];
    alertForMiscreant(people);
    expect(spy).toHaveBeenCalledTimes(0);
    spy.mockRestore();
  });

  ........
});

2. 元の関数をコピーして新しい関数を作成

ますは、元の関数をまるまるコピーしてきて、名前を変更します。
大げさかなと思うかもしれませんが、テストを繰り返しながら確実に一歩ずつリファクタをすすめる上でこの手順は無視できません。

index.js

import { setCombatAlerm } from "./setCombatAlerm";

export function alertForMiscreant(people) {
  for (const p of people) {
    if (p === "Don") {
      setAlerms();
      return "Don";
    }
    if (p === "John") {
      setAlerms();
      return "John";
    }
  }
  return "";
}

// まるまるコピーして、名前だけ変更
export function findMiscreant(people) {
  for (const p of people) {
    if (p === "Don") {
      setAlerms();
      return "Don";
    }
    if (p === "John") {
      setAlerms();
      return "John";
    }
  }
  return "";
}

3. 新しい関数をから、副作用の部分を取り除いて問い合わせ専用の関数とする

さらにここから、更新に係る処理を削除して、副作用を消します。

index.js

....
export function findMiscreant(people) {
  for (const p of people) {
    if (p === "Don") {
-     setAlerms();
      return "Don";
    }
    if (p === "John") {
-      setAlerms();
      return "John";
    }
  }
  return "";
}

4. 元の関数の問い合わせ部分を、新しい関数で置き換える

alertForMiscreant に入っていた問い合わせ部分を、findMiscreant で置き換えます。
問い合わせ部分が分離されていることがわかります。

index.js

import { setCombatAlerm } from "./setCombatAlerm";

export function alertForMiscreant(people) {
  if (findMiscreant(people) !== '') {
    setAlerms();
}

export function findMiscreant(people) {
  for (const p of people) {
    if (p === "Don") {
      return "Don";
    }
    if (p === "John") {
      return "John";
    }
  }
  return "";
}

「問い合わせと更新の分離」の目的は果たせましたが、もう少しだけリファクタを進めておきます。
まだまだやりたい修正はたくさんですが、一旦以下で止めておきます。

index.js

import { setCombatAlerm } from "./setCombatAlerm";

export function alertForMiscreant(people) {
  if (someMiscreant(people)) {
    setAlerms();
}

export function someMiscreant(people) {
  return people.some((person) => {
    return ["Don", "John"].includes(person);
  });
}

関数が増えるとともに
alertForMiscreant の戻り値の型が string -> void
someMiscreant の戻り値の型が string -> boolean
に変更されましたがテストとステップバイステップの修正のおかげで安全にリファクタをすることができました

まとめ

正直「問い合わせと更新の分離」は日々自然に行っているなと感じる方も多いのではないでしょうか？
ただ、ここまで確実に一歩ずつやっていたかと言われるともう少し荒いステップだったなと、振り返るきっかけになりました
明日は、もう一歩きれいなコードを書きたいですね

参考

• リファクタリング 既存のコードを安全に改善する（第2版）
• jest spyOn