6
\$\begingroup\$

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:

  1. It is assigned to a server.
  2. It can be in state unbound (implies unprotected), bound (implies protected), reserved.
  3. It has several firewall rules which should be automatically activated/deactivated when binding/unbinding.
  4. It can be migrated from/to another server.
  5. 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_server

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

  def bind!
    wait!(:bound)
  end

  def unbind!
    wait!(:unbound)
  end

  def migrate!(new_server)
    restore_state do
      unbind!
      self.server = new_server
    end
  end

  def refresh!
    return if reserved?
    self.state = self.class.new.state # set to initial state
    @refreshing = true
    nil while cycle
    @refreshing = false
  end

  def restore_state
    old_state = state
    yield
    wait!(old_state)
  end

  def wait!(desired_state)
    cycle! while !state?(desired_state)
  end

  def refreshing?
    @refreshing
  end

  def activate_firewall_rules
    puts "activate_firewall_rules..."
    @is_protected = true
  end

  def deactivate_firewall_rules
    puts "deactivate_firewall_rules..."
    @is_protected = false
  end

  def bind_to_server
    puts "bind_to_server..."
    @is_bound = true
  end

  def unbind_from_server
    puts "unbind_from_server..."
    @is_bound = false
  end

  def is_protected?
    puts "is_protected: #{@is_protected}"
    @is_protected
  end

  def is_unprotected?
    !is_protected?
  end

  def is_bound?
    puts "is_bound: #{@is_bound}"
    @is_bound
  end

  def is_unbound?
    !is_bound?
  end
end

The public API should be something like this:

bind! (bring up firewall and bind address to server)
unbind! (unbind address to server and bring down firewall
refresh! (get real/external state and update internal state)
migrate! (assign to new server, retain state)

Since this is my very first state machine I thought I'd like to ask what you think about it. Do you think using a state machine makes sense here at all? Am I using it correctly or am I doing something wrong? What and why?

\$\endgroup\$

1 Answer 1

2
\$\begingroup\$

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.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.