How to handle a strategy with dependency injection

Question

I have to create an order system which has a local and external storage. The local storage should always be used up first and if there is still anything missing the rest should ordered by the external storage.

Basically I have 4 possible cases:

1 Order all items from local storage
2 Order items mixed from local storage and external storage
3 Order all items from external storage
4 Order not possible

Now I decided to make an interface for the 3 valid cases the Interface is quite simple:

public interface Reservator{
  boolean reserveItem(Article article, amount);
}

Now for choosing which strategy I use the following interface:

public interface ReservationContext{
  boolean setAmount(int amount);
  boolean reserveItem(Article article);
}

For which the implementation looks roughly like this:

@Singleton
public ReservationHandler implements ReservationContext{
  private int reservationAmount=0;
  private Reservator reservator;
  private LocalStorage local;
  private ExternalStorage external;

  @Inject
  public ReservationHandler(LocalStorage local, ExternalStorage external){
    this.local = local;
    this.external = external;

  }
  @Override
  public boolean setAmount(int amount){
    if(amount == 0){
      return false;
    }
    if(local.getStorage > amount){
      reservator = new LocalReservator(//all dependencies);
    }
    else if(local.getStorage ==0 && external.getStorage > amount){
      reservator = new ExternalReservator(//all dependencies);
    }
    else if((local.getStorage + external.getStorage) > amount){
      reservator = new MixedReservator(//all dependencies);
    }
    else{
      return false;
    }
    reservationAmount = amount;
    return true;
  }

  @Override
  public boolean reserveItem(Article article){
    return reservator.reserveItem(article, amount);
  }

}

Now I wonder how I could handle the three reservation-strategies (LocalReservator, ExternalReservator and MixedReservator) over dependency injection instead of instancing to allow easier testing.

I know that I could bind all implementations of the Reservator-Interace and get a List when injecting. However it is not totally clear to me how I can choose the correct one easily. What is the best practice in my case?

Also if the selection of the correct strategy could be handled better I'm open to suggestions. Thanks for your time!

Answer 1

Your ReservationHandler is currently responsible for doing at least three things: Creating the Reservator instances, calculating which Reservator to use, and acting as a ReservationContext. Though this code isn't too bad as it is, we can easily let Guice handle creating the Reservator instances--that's its job, after all, to encapsulate dependency creation--and consider extracting out the creation of the correct Reservator. This will also insulate you if the number of Reservators grows, or if ReservationContext grows to have a clearer scope. (Right now it just looks like a wrapper interface.)

Rather than a List, what you're looking for is a set of Providers:

private final Provider<LocalReservator> localReservatorProvider;
private final Provider<ExternalReservator> externalReservatorProvider;
private final Provider<MixedReservator> mixedReservatorProvider;

You'd set these by receiving them in your constructor from Guice (which can inject a Provider<T> automatically for any T you've bound in your graph), and then you can call them in your method.

if(local.getStorage > amount){
  reservator = localReservatorProvider.get();  // no dependencies!
}
else if(local.getStorage ==0 && external.getStorage > amount){
  reservator = externalReservatorProvider.get();
}
else if((local.getStorage + external.getStorage) > amount){
  reservator = mixedReservatorProvider.get();
}
else{
  return false;
}

Should you do this? Right now you haven't shown us the dependencies for your Reservator instances, or whether they're likely to change. If they aren't, and/or the list is short, you might stick to using new; if the list is long or likely to change, it makes a lot of sense to inject Providers. It would also make sense to inject Providers if your instances are hard to work with in tests, because you could use Providers.of() with a mock to replace a real LocalReservator with a simple deterministic test double (and so on for the rest).

If you wanted to build on this, you could also involve Guice multibindings (which lets you create a Set or Map in Guice in a distributed way, one binding at a time) or Assisted Injection (which would let you pass constructor arguments during your Reservator creation, using a Factory you define instead of Guice's Provider). Neither is immediately applicable, but both are worth learning about.

From here, if your setAmount logic is complicated enough to extract, you might also extract it to a one-method class ReservatorFactory that selects which Reservator to return. That way, your ReservationHandler can be left to its own responsibilities, and can be tested without exercising your specific reservation logic. (If, of course, that's the only thing you want ReservationHandler to do, then by all means leave that implementation where it is. Otherwise you could refactor ReservationHandler into an empty shell!)

p.s. Jack is correct in the comments that it can be very dangerous to have this marked @Singleton while it keeps state for individual reservations. You can drop the @Singleton annotation and Guice will create a new instance each time; that's Guice's default behavior.