Code review of an Order object implemented as a state machine

Let me show you an example piece of code - an Order class. This class is used in our sample DDD/CQRS/ES application. We’re in the process of improving this app so this is a good opportunity to document certain opinions and changes.

module Ordering
  class Order
    include AggregateRoot

    AlreadySubmitted = Class.new(StandardError)
    AlreadyPaid = Class.new(StandardError)
    NotSubmitted = Class.new(StandardError)
    OrderHasExpired = Class.new(StandardError)
    MissingCustomer = Class.new(StandardError)

    def initialize(id)
      @id = id
      @state = :draft
    end

    def submit(order_number, customer_id)
      raise AlreadySubmitted if @state.equal?(:submitted)
      raise OrderHasExpired  if @state.equal?(:expired)
      raise MissingCustomer unless customer_id
      apply OrderSubmitted.new(data: {order_id: @id, order_number: order_number, customer_id: customer_id})
    end

    def confirm(transaction_id)
      raise OrderHasExpired if @state.equal?(:expired)
      raise NotSubmitted unless @state.equal?(:submitted)
      apply OrderPaid.new(data: {order_id: @id, transaction_id: transaction_id})
    end

    def expire
      raise AlreadyPaid if @state.equal?(:paid)
      apply OrderExpired.new(data: {order_id: @id})
    end

    def add_item(product_id)
      raise AlreadySubmitted unless @state.equal?(:draft)
      apply ItemAddedToBasket.new(data: {order_id: @id, product_id: product_id})
    end

    def remove_item(product_id)
      raise AlreadySubmitted unless @state.equal?(:draft)
      apply ItemRemovedFromBasket.new(data: {order_id: @id, product_id: product_id})
    end

    def cancel
      raise OrderHasExpired if @state.equal?(:expired)
      raise NotSubmitted unless @state.equal?(:submitted)
      apply OrderCancelled.new(data: {order_id: @id})
    end

    on OrderSubmitted do |event|
      @customer_id = event.data[:customer_id]
      @number = event.data[:order_number]
      @state = :submitted
    end

    on OrderPaid do |event|
      @state = :paid
    end

    on OrderExpired do |event|
      @state = :expired
    end

    on OrderCancelled do |event|
      @state = :cancelled
    end

    on ItemAddedToBasket do |event|
    end

    on ItemRemovedFromBasket do |event|
    end
  end
end

As always, this class was nice and simple at the beginning, but over time it grew and became less readable.

There are now several responsibilities of this class. Some of those I will leave for another discussion (like coupling the domain code with event code). Today I want to focus on the concept of a state machine.

State machine

This Order object has now 5 possible states. How come it grew to such size? As always - one by one. 5 is probably still OKish but we can imagine what can happen if we extend it even more.

It’s worth noting that this class uses AggregateRoot which helps in the event sourcing part of this object.

What are the requirements for this state machine?

More or less this:

Order draft submitted paid expired cancelled
draft
submitted
paid
expired
cancelled

Draft is the initial state. The happy path then switches to submitted, then to paid. The less happy paths include expired and cancelled, both are leaf states.

Challenges

The challenge with state machines is that it’s not easy to represent them in code in a readable manner. Whenever the number of states and transitions grows it’s becoming harder to read such code.

What makes a state machine?

State machines consist of states and transitions. Somehow we need to represent them in the code. In this implementation, we put the transition as the main “dimension”. The method names show the possible transitions. However, they show possible transitions for all the states. This leads to a problem, that in each of those methods we now need to “disable” the impossible transitions. We could do it with just an early return in such cases without using exceptions. The problem with this code is that it’s hard to easily say, what is the possible flow in this state machine. The code is infected with other responsibilities which make it all less readable.

Exceptions

BTW, why do we use exceptions here?

Because one responsibility of this object is to communicate “WHY” a certain change is not possible. An early return only communicates a boolean information - possible or not. A custom exception brings more context.

Possible improvements

What are the possible directions of improvement here?

  • Reduce the size of this state machine
  • Decouple an object to explain why a change is not possible from the code which just says it’s not possible
  • Extract a new object per a possible state
  • Extract the event logic out of this class

The first direction is the most tempting here. Reducing the size via reducing the number of possible states would help. It would help as it decreases the scope of other problems too. That’s the direction that is most beneficial - it does improve the root of the problem and thus reduces other problems.

How can we reduce the number of states here? Stay tuned :)

Would you like to learn how to write such blogposts? Join our FREE dev blogging conference - learn how to write and promote blogposts without sacrificing the coding time. It's 3 days, 4 talks - April 14-16th (Wed-Fri). Register to grab your spot.

You might also like