Guiceへのリファクタリング

ヒドイコード

こういうのどこにでもあるでしょ！静的なユーティリティメソッドがいろんなものに依存していて、それもまた静的ときたもんだ。

public class PizzaUtilities {
  private static final int TIME_TO_PREPARE = 6;
  private static final int MAX_DISTANCE = 20;

  public static Order createOrder(List<PizzaSpec> pizzas, Customer customer) {
    /** 訳注：配達すべき方向を取得する */
    Directions directions = Geography.getDirections(
      PizzaStore.getStoreAddress(), customer.getDeliveryAddress());

    /** 訳注：方向がわかんない、あるいは遠すぎる場合は例外 */
    if (directions == null || directions.getLengthInKm() > MAX_DISTANCE) {
      throw new InvalidOrderException("Cannot deliver to , " +
          customer.getDeliveryAddress());
    }

    /** 訳注：到着時刻を計算する。料理時間、オーブンの時間、配達値までの時間 */
    int arrivalTime = TIME_TO_PREPARE
        + Oven.getCurrentOven().schedule(TIME_TO_PREPARE, pizzas)
        + directions.estimateTravelTime();

    /** 訳注：納品書を作成 */
    Invoice invoice = Invoice.create(pizzas, directions.getLengthInKm());

　　　　/** 訳注：注文オブジェクトを作成 */
    return new Order(pizzas, invoice, arrivalTime, customer, directions);
  }
}

なんでこれが気に入らないのか？

Geography, PizzaStore, Oven, Invoiceクラスに静的に依存しているもんで、こいつらもひっくるめてテストをやんなきゃんらない。結果、

• PizzaStoreの初期化が遅かったりすると、テストも待たされてしまう。

• このメソッドをテストするためには、Geograpyのコードが必要だ。もし別のチームがそれを作っている最中だとしても。
• テストのsetUp()の中で、Oven.setCurrentOven()をセットすることを覚えておかければ。そうしないとテストは失敗してしまう。
• もし、Invoice.create()が外部サービス、例えば支払処理サービスなんかに依存してたりすると、そのサービスがなきゃテストできないじゃん。

同じメソッドの静的でないバージョン

静的でないバージョンを作ってみよう。

public class PizzaServices {
  public Order createOrder(List<PizzaSpec> pizzas, Customer customer) {
    return PizzaUtilities.createOrder(pizzas, customer);
  }
}

静的な呼び出しを静的でない呼び出しに置き換える

Now wherever I have calling code that was calling PizzaUtilities, I can replace it with an injected instance of PizzaServices. For example, this:

class OrderPizzaAction {
  public void order(HttpSession session) {
    Customer customer = session.getCurrentCustomer();
    PizzaUtilities.createOrder(getPizzaSpecs(), customer);
  }
  ...
}

becomes this:

class OrderPizzaAction {
  private final PizzaServices pizzaServices;
  @Inject
  OrderPizzaAction(PizzaServices pizzaServices) {
    this.pizzaServices = pizzaServices;
  }

  public void order(HttpSession session) {
    Customer customer = session.getCurrentCustomer();
    pizzaServices.createOrder(getPizzaSpecs(), customer);
  }
}

Notice that this required me to change the constructor for OrderPizzaAction. If you'd like to postpone changing the constructor until later, you can still use PizzaServices by statically-injecting the PizzaServices instance:

class OrderPizzaAction {
  @Inject public static PizzaServices pizzaServices;

  public void order(HttpSession session) {
    Customer customer = session.getCurrentCustomer();
    pizzaServices.createOrder(getPizzaSpecs(), customer);
  }
}

And then in our module, we prepare the static injection:

class PizzaModule extends AbstractModule {
  protected void configure() {
    requestStaticInjection(OrderPizzaAction.class);
  }
}

Static injection is very helpful as a transitional aide while refactoring from static to non-static code.

One benefit of this work is already available. OrderPizzaAction can now be tested without the Geography etc., by passing in a mock subclass of PizzaServices that overrides createOrder.

Moving logic to the non-static version

Next, let's move the implementations logic from PizzaUtilities to PizzaServices. We'll leave a forwarding method in place in PizzaUtilities so we don't have to update all the callers right away:

public class PizzaUtilities {
  @Inject public static PizzaServices pizzaServices;

  public static Order createOrder(List<PizzaSpec> pizzas, Customer customer) {
    return pizzaServices.createOrder(pizzas, customer);
  }
}

public class PizzaServices {
  private static final int TIME_TO_PREPARE = 6;
  private static final int MAX_DISTANCE = 20;

  public Order createOrder(List<PizzaSpec> pizzas, Customer customer) {
    Directions directions = Geography.getDirections(
      PizzaStore.getStoreAddress(), customer.getDeliveryAddress());
    ...
    return new Order(pizzas, invoice, arrivalTime, customer, directions);
  }
}

Notice that we'll used static injection to make our PizzaServices instance available to PizzaUtilities, so we'll need to update the module to prepare that:

class PizzaModule extends AbstractModule {
  protected void configure() {
    requestStaticInjection(OrderPizzaAction.class);
    requestStaticInjection(PizzaUtilities.class);
  }
}

Injecting the non-static version

Now that PizzaUtilities is injected, we can start to inject its dependencies into it. The low-hanging fruit is the Oven.getCurrentOven() singleton. We'll bind that in the module and then we can inject it!

public class PizzaServices {
  private final Oven currentOven;

  @Inject
  public PizzaServices(Oven currentOven) {
    this.currentOven = currentOven;
  }

  public Order createOrder(List<PizzaSpec> pizzas, Customer customer) {
    ...
    int arrivalTime = TIME_TO_PREPARE
        + currentOven.schedule(TIME_TO_PREPARE, pizzas)
        + directions.estimateTravelTime();
    ...
  }
}

and then in the Module:

class PizzaModule extends AbstractModule {
  protected void configure() {
    requestStaticInjection(OrderPizzaAction.class);
    requestStaticInjection(PizzaUtilities.class);
    bind(Oven.class).toProvider(new Provider() {
      public Oven get() {
        return Oven.getCurrentOven();
      }
    });
  }
}

This means that whenever an Oven instance is injected, it'll use the old Oven.getCurrentOven() method to get it. Later on, we'll be able to remove that method as well.

Now we can test PizzaServices without a particular Oven instance prepared in advance. In a future post, I'll demonstrate how to remove the remaining static calls.