views:

428

answers:

5

Guys,

I have some doubts about the following implementation of the state pattern:

I have an Order object. For simplicity, let's suppose it has a quantity, productId, price and supplier. Also, there are a set of known states in wich the order can transition:

  • state a: order is new, quantity must be > 0 and must have productId. Price and supplier are not yet assigned.
  • state b: someone checks the order. It can only be cancelled, or have the supplier assigned.
  • state c: supplier can only fill in the price to be charged to the client.
  • State d: the order is cancelled.

1) Order.isValid() changes between states. Ie, in state a some operations cannot be done. So, they look like:
void setQuantity(int q) {
if (_state.canChangeQuantity()) this.quantity = q;
else throw an exception.
}
Is this right, or should i get each state to implement setQuantity operation? In that case, where will be stored the value? In the order, or the state? In the latter case, i will have to copy the data in each state transition?

2) orderProcessor.process(order) is an object that checks order.IsValid, transition the order to some state, saves it to the db and perform some custom actions (in some states, admin is notified, in others the client, etc). I have one for each state.
In StateAOrderProcessor, the person that checks the order is notified via email, and the order is transitioned to state b.
Now, this pushes the state transitions outside the Order class. That means that Order has a 'setState' method, so each processor can change it. This thing to change the state from the outside does not sounds nice. Is thas right?

3) Ahother option is to move all the validation logic to each state's processor, but now i have to track when a order's quantity was changed to see if that operation is valid in the current state. That leaves the order kind of anemic to me.

What do you think guys? Can you give me some advice to desing this thing better?

Thank you very much.

Nick

A: 

Do you have several different classes, one per state.

BaseOrder {
    //  common getters
    // persistence capabilities
}

NewOrder extends BaseOrder {
    // setters
    CheckingOrder placeOrder();
} 

CheckingOrder extends BaseOrder {
     CancelledOrder cancel();
     PricingOrder assignSupplier();
}

and so on. The idea is that code that needs orders in a particular state only gets objects of the right class, so no state checks are needed. Code that just wants to operate on orders in any state case use the BaseClass.

djna
So you wouldn't use the state pattern? How would you accomplish some of the sample requirements described? I cannot see the picture. Thanks.
The essence of the state pattern (at least as I read http://en.wikipedia.org/wiki/State_pattern) is that the same capabilities are available in each state, and the use of inheritence for the different states makes perfect polymorphic sense. You also want extra behaviours specific to each state. It makes sense to add them to those subclasses. I don't see any violation of the state concept in the approach I describe.
djna
@djna: As you describe it, you seem to be missing out the context object. The point of the state pattern is it allows you to transition a live object (the context) from one state to another without copying all the data to a new object. To do this you put the data in the context class and then have that delegate the operations to a state class. This allows you to change the state after object creation. Without the context class, you just have a plain old inheritance hierarchy rather than the state pattern.
Martin Brown
@Martin Brown yes, you're right, thanks. I was busy focusing on the client's view, where I think use of separate State Classes is reasonable. The pseudo code as I present it loses the context, which avoids copying data at state transitions.
djna
A: 

Changing the current state object can be done directly from a state object, from the Order and even OK from an external source (processor), though unusual.

According to the State pattern the Order object delegates all requests to the current OrderState object. If setQuantity() is a state-specific operation (it is in your example) then each OrderState object should implement it.

So, if setQuantity is a state-specific operation, the quantity will be saved in the current state. When the Order moves to a new state, i'll not only could need the Context (Order) but the old state too, to get the quantity. Is that right? Are there others ways to do this?Thankx.
I'd say the quantity is an attribute of the Order itself. The OrderState classes encapsulate state-specific behaviour. You will have the following classes: Order, OrderStateA, OrderStateB, OrderStateC, OrderStateD.In your a. and c. states the order quantity can be changed, in the other states can not. You can throw an exception from the setQuantity() method in these cases.
That's where my question is going. If in each OrderState i have a setQuantity method, i'll be storing data in each state right. Now, i could store the quantity in each state (data) or i could check if i can set the quantity in the order (from the state, behaviour) and raise an exception. What is the correct way to go? (sorry if my comment was unclear)
I would store the quantity in the Order, and raise an exception if the caller wants to change the quantity in a state when he is not allowed to.
A: 

In order for the state pattern to work, the context object has to expose an interface that the state classes can use. At a bare minimum this will have to include a changeState(State) method. This I'm afraid is just one of the limitations of the pattern and is a possible reason why it is not always useful. The secret to using the state pattern is to keep the interface required by the states as small as possible and restricted to a tight scope.

(1) Having a canChangeQuantity method is probably better than having all states implement a setQuantity. If some states are doing something more complex than throwing an exception, this advice may not follow.

(2) The setState method is unavoidable. It should, however, be kept as tightly scoped as possible. In Java this would probably be Package scope, in .Net it would be Assembly (internal) scope.

(3) The point about validation raises the question of when you do validation. In some cases, it is sensible to allow the client to set properties to invalid values and only validate them when you do some processing. In this case each state having an 'isValid()' method that validates the whole context makes sense. In other cases you want a more immediate error in which case I would create an isQuantityValid(qty) and isPriceValid(price) that would be called by the set methods before changing the values, if they return false throw an exception. I've always called these two Early and Late Validation and it is not easy to say which you need without knowing more about what you are up to.

Martin Brown
A: 

I would store information in the Order class, and pass a pointer to the Order instance to the state. Something like this:


class Order {
  setQuantity(q) {
    _state.setQuantity(q);
  } 
}

StateA {
  setQuantity(q) {
    _order.q = q;
  }
}

StateB {
  setQuantity(q) {
    throw exception;
  }
}

agateau
+1  A: 

This is an ideal scenario for the state pattern.

In the State pattern, your state classes should be responsible for transitioning state, not just checking the validity of the transition. In addition, pushing state transitions outside of the order class is not a good idea and goes against the pattern, but you can still work with an OrderProcessor class.

You should get each state class to implement the setQuantity operation. The state class should implement all methods that may be valid in some states but not in others, whether or not it involves a change of state.

There is no need for methods such as canChangeQuantity() and isValid() - the state classes ensure your order instances are always in a valid state, because any operation that is not valid for the current state will throw if you try it.

Your Order class's properties are stored with the order, not the state. In .Net, you'd make this work by nesting your state classes inside the Order class and providing a reference to the order when making calls - the state class will then have access to the order's private members. If you're not working in .Net, you need to find a similar mechanism for your language - for example, friend classes in C++.

A few comments on your states and transitions:

  • State A notes that the order is new, quantity is >0 and it has a product id. To me, this means you're either providing both of those values in the constructor (to ensure your instance starts off in a valid state, but you wouldn't need a setQuantity method), or you need an initial state which has an assignProduct(Int32 quantity, Int32 productId) method that will transition from the initial state to state A.

  • Similarly, you may want to consider an end state to transition to from state C, after the supplier has filled in the price.

  • If your state transition requires the assignment of two properties, you may want to consider using a single method that accepts both properties by parameter (and not setQuantity followed by set setProductId), to make the transition explicit.

  • I would also suggest more descriptive state names - for example, instead of StateD, call it CanceledOrder.

Here's an example of how I'd implement this pattern in C#, without adding any new states:

 public class Order
 {
  private BaseState _currentState;

  public Order(
   Int32 quantity,
   Int32 prodId)
  {
   Quantity = quantity;
   ProductId = prodId;
   _currentState = new StateA();
  }

  public Int32 Quantity
  {
   get; private set;
  }

  public Int32 ProductId
  {
   get; private set;
  }

  public String Supplier
  {
   get; private set;
  }

  public Decimal Price
  {
   get; private set;
  }

  public void CancelOrder()
  {
   _currentState.CancelOrder(this);
  }

  public void AssignSupplier(
   String supplier)
  {
   _currentState.AssignSupplier(this, supplier);
  }

  public virtual void AssignPrice(
   Decimal price)
  {
   _currentState.AssignPrice(this, price);
  }


  abstract class BaseState
  {
   public virtual void CancelOrder(
    Order o)
   {
    throw new NotSupportedException(
     "Invalid operation for order state");
   }

   public virtual void AssignSupplier(
    Order o, 
    String supplier)
   {
    throw new NotSupportedException(
     "Invalid operation for order state");
   }

   public virtual void AssignPrice(
    Order o, 
    Decimal price)
   {
    throw new NotSupportedException(
     "Invalid operation for order state");
   }
  }

  class StateA : BaseState
  {
   public override void CancelOrder(
    Order o)
   {
    o._currentState = new StateD();
   }

   public override void AssignSupplier(
    Order o, 
    String supplier)
   {
    o.Supplier = supplier;
    o._currentState = new StateB();
   }
  }

  class StateB : BaseState
  {
   public virtual void AssignPrice(
    Order o, 
    Decimal price)
   {
    o.Price = price;
    o._currentState = new StateC();
   }
  }

  class StateC : BaseState
  {
  }

  class StateD : BaseState
  {
  }
 }

You can work with your order processor classes, but they work with the public methods on the order class and let the order's state classes keep all responsibility for transitioning state. If you need to know what state you're in currently (to allow the order processor to determine what to do), you might add a String Status property on the order class and on BaseState, and have each concrete state class return its name.

Remi Despres-Smyth