Usage of state machine to model lifetime of an IP address

I need to model an IP address and thought using a state machine (the state_machine gem) would be a good idea.

An IP has the following characteristics:

• It is assigned to a server.

• It can be in state unbound (implies unprotected), bound (implies protected), reserved.

• It has several firewall rules which should be automatically activated/deactivated when binding/unbinding.

• It can be migrated from/to another server.

• All operations, such as (de)activating firewall rules and (un)binding, may fail or externally change their state (consider a machine reboot).

Here is what I came up with so far:

```
require 'rubygems'
require 'state_machine'
require 'awesome_print'

class Ip
state_machine :state, :initial => :unbound do
# idle states, i.e. only an event (either external or internal) will trigger a transition.
state :unbound, :bound, :reserved

# transitionary states, we expect a state change after a certain period of time.
state :protected, :unprotected

event :cycle do
transition :unbound => :protected
transition :protected => :bound
transition :bound => :unprotected
transition :unprotected => :unbound
end

event :reserve do
transition :unbound => :reserved
end

event :release do
transition :reserved => :unbound
end

before_transition any => any, :if => :refreshing? do |ip, transition|
puts "refreshing, before_transition -> state is: #{transition.from}, state will be: #{transition.to}"
throw :halt unless ip.send("is_#{transition.to}?")
end

before_transition any => any do |ip, transition|
puts "before_transition -> state is: #{transition.from}, state will be: #{transition.to}"
end

before_transition any => :protected, :do => :activate_firewall_rules
before_transition any => :unprotected, :do => :deactivate_firewall_rules

before_transition any => :bound, :do => :bind_to_server
before_transition any => :unbound, :do => :unbind_from

Better late than never.

Yes, a state machine makes sense here. You've chosen good names and your coding style is good.

A few comments:

The "puts" lines appear to be for debugging. If that is so, and they are going to remain in the code, I would consider writing to an IO object rather than $stdout. This will make tests much nicer to run, since when passing, you won't be bothered by all the debugging information. For example:

attr_accessor :debug_io
...
after_transition any => any do |ip, transition|
  @debug_io.puts "after_transition -> state was: #{transition.from}, state is: #{transition.to}"
end

And because you want it to still work even if the caller has not set debug_io, give it a default. "No output" is a good default:

class NullIO
  def puts ; end
end

def initialize
  super
  @debug_io = NullIO.new
end

The return value of is_protected? depends only upon the state, being true if in the bound or protected state. Remove:

before_transition any => :protected, :do => :activate_firewall_rules
before_transition any => :unprotected, :do => :deactivate_firewall_rules

and the methods they call, and change is_protected? to:

def is_protected?
  [:protected, :bound].include?(state)
end

Similarly, remove:

before_transition any => :bound, :do => :bind_to_server
before_transition any => :unbound, :do => :unbind_from_server

and the methods they call, and change is_bound? to:

def is_bound?
  [:bound, :unprotected].include?(state)
end

Ruby predicates usually leave off the "is_" prefix. Consider changing is_bound? to bound? and is_protected? to protected?. The is_ prefix comes from languages which do not allow question marks in identifiers.

Mark as "private" methods which are for the use of the class only, and should not be called by the class's user. This will help prevent accidental misuse of the class, but more importantly, document the class's public signature, which helps in understanding and is a big help when refactoring. Methods such as bind_to_server and unbind_from_server are private (or were, before they were removed).

It's not clear what this line is for:

throw :halt unless ip.send("is_#{transition.to}?")

If it's not for debugging, it seems like an odd bit of hidden coupling between the class and the rest of the program; I would try to find another way to do this that makes it clearer why the :halt is being thrown.

StackExchange Code Review Q#8988, answer score: 2