6
\$\begingroup\$

I've asked Code Climate to generate metrics for the ftpd Ruby gem. It correctly identified the God class; I know what to do about that. But one of the smaller classes has me stumped. This is telnet.rb:

# -*- ruby encoding: us-ascii -*-

module Ftpd

  # Handle the limited processing of Telnet sequences required by the
  # FTP RFCs.
  #
  # Telnet option processing is quite complex, but we need do only a
  # simple subset of it, since we can disagree with any request by the
  # client to turn on an option (RFC-1123 4.1.2.12).  Adhering to
  # RFC-1143 ("The Q Method of Implementing TELNET Option Negiation"),
  # and supporting only what's needed to keep all options turned off:
  #
  # * Reply to WILL sequence with DONT sequence
  # * Reply to DO sequence with WONT sequence
  # * Ignore WONT sequence
  # * Ignore DONT sequence
  #
  # We also handle the "interrupt process" and "data mark" sequences,
  # which the client sends before the ABORT command, by ignoring them.
  #
  # All Telnet sequence start with an IAC, followed by at least one
  # character.  Here are the sequences we care about:
  #
  #     SEQUENCE             CODES
  #     -----------------    --------------------
  #     WILL                 IAC WILL option-code
  #     WONT                 IAC WONT option-code
  #     DO                   IAC DO option-code
  #     DONT                 IAC DONT option-code
  #     escaped 255          IAC IAC
  #     interrupt process    IAC IP
  #     data mark            IAC DM
  #
  # Any pathalogical sequence (e.g. IAC + \x01), or any sequence we
  # don't recognize, we pass through.

  class Telnet

    # The command with recognized Telnet sequences removed

    attr_reader :plain

    # Any Telnet sequences to send

    attr_reader :reply

    # Create a new instance with a command that may contain Telnet
    # sequences.
    # @param command [String]

    def initialize(command)
      telnet_state_machine command
    end

    private

    module Codes
      IAC  = 255.chr    # 0xff
      DONT = 254.chr    # 0xfe
      DO   = 253.chr    # 0xfd
      WONT = 252.chr    # 0xfc
      WILL = 251.chr    # 0xfb
      IP   = 244.chr    # 0xf4
      DM   = 242.chr    # 0xf2
    end
    include Codes

    def telnet_state_machine (command)
      @plain = ''
      @reply = ''
      state = :idle
      command.each_char do |c|
        case state
        when :idle
          if c == IAC
            state = :iac
          else
            @plain << c
          end
        when :iac
          case c
          when IAC
            @plain << c
            state = :idle
          when WILL
            state = :will
          when WONT
            state = :wont
          when DO
            state = :do
          when DONT
            state = :dont
          when IP
            state = :idle
          when DM
            state = :idle
          else
            @plain << IAC + c
            state = :idle
          end
        when :will
          @reply << IAC + DONT + c
          state = :idle
        when :wont
          state = :idle
        when :do
          @reply << IAC + WONT + c
          state = :idle
        when :dont
          state = :idle
        else
          raise "Unknown state #{state.inspect}"
        end
      end
    end

  end
end

Code Climate does not like the complexity of #telnet_state_machine. I agree, but I don't know how to reduce the complexity without also making the state machine harder to follow. State machines never seem to be all that readable as it is. What would you suggest?

Note: If you want to try some refactoring, this class has rspec test coverage. Just do "git clone" of the ftpd project.

\$\endgroup\$
  • 1
    \$\begingroup\$ what about using a "real" state machine instead of a switch implementation ? If you don't want to use a gem like state_machine you can always try to use composition - i'll try to come up with something if i have time \$\endgroup\$ – m_x Oct 2 '13 at 19:33
  • \$\begingroup\$ @m_x I like the idea of trying the state_machine gem. I'll try it and post the result here, if nobody else has. \$\endgroup\$ – Wayne Conrad Oct 4 '13 at 15:20
  • \$\begingroup\$ In any case, are you looking for contributors on your gem ? I never contributed to any open-source project and i'd like to give it a try. I'm willing to do anything you throw at me, even if it's only doc improvement \$\endgroup\$ – m_x Oct 9 '13 at 20:42
  • \$\begingroup\$ @m_x, Sure! The project is on github (see link in question). My email address is in the README. \$\endgroup\$ – Wayne Conrad Oct 9 '13 at 21:18
3
\$\begingroup\$

EDIT

the more I think about it, the more it seems obvious that a parser would be more adapted to this problem than a state machine. Unfortunately, I don't know much about writing parsers...

ORIGINAL ANSWER

Maybe you could try to create one class per state :

class TelnetState

  include Telnet::Codes
  attr_reader :plain, :reply

  def initialize( plain = '', reply ='' )
    @plain = plain
    @reply = reply
  end

  def accept_char( char )
    raise "Abstract Method - not implemented"
  end

end

class TelnetIDLEState < TelnetState

  def accept_char( char )
    if char == IAC 
      TelnetIACState.new( plain, reply )
    else
      @plain << char
      self        
    end
  end

end

class TelnetIACState < TelnetState

   def accept_char( char )
     update_plain!( char )
     next_state( char )
   end

   private

   def update_plain!( char )
     return if [WILL, WONT, DO, DONT, IP,DM].include? char
     @plain << char == IAC ? char : IAC + char
   end

   def next_state( char ) 
     next = case char
              when WILL then TelnetWILLState
              when WONT then TelnetWONTState
              when DO   then TelnetDOState
              when WONT then TelnetDONTState
            end
     next ? next.new( plain, reply ) : self
   end

 end

... and so on, then :

class Telnet

  def telnet_state_machine( command )
    state = command.each_char.inject( TelnetIDLEState.new ) do |state, char|
              state.accept_char( char )
            end
    @plain, @reply = state.plain, state.reply
  end

end

Of course, there is certainly a way to improve that... just an idea. (I'm not really familiar with telnet protocol, either - so I did this a bit blindfolded)

|improve this answer|||||
\$\endgroup\$
  • \$\begingroup\$ Thank you for the answer. I'm not keen on the class-per-state idea because it spreads the state machine out over multiple pages. State machines in code are difficult to read, being a 1D representation of a 2D structure; when the representation no longer fits on a screen, they get much harder to understand. The idea of using a parser is interesting! I suspect a general purpose parser such as parslet is too heavy a tool to apply to this problem, but I'll keep the idea in mind. \$\endgroup\$ – Wayne Conrad Oct 4 '13 at 15:17
  • \$\begingroup\$ Agreed, a state machine can become very hard to understand - when the pattern does not naturally adhere to its subject, as it is the case here. In some other cases it feels logic and natural. As i said, I don't really know how to build a parser; however the problem just calls it pretty clearly. Think of it : you have a string that is just a sequence of symbols that express commands to perform in a certain order... \$\endgroup\$ – m_x Oct 4 '13 at 17:09
  • \$\begingroup\$ just out of curiosity, I just checked the source of core's Telnet class... just to find out it's far more complex (to say the least) than your implementation. Do you have reasons not to use core ? \$\endgroup\$ – m_x Oct 4 '13 at 17:20
  • \$\begingroup\$ also, if it's a server you need, you might want to take a look at this \$\endgroup\$ – m_x Oct 4 '13 at 17:24
  • \$\begingroup\$ btw, thanks for pointing out parslet, seems pretty neat ! \$\endgroup\$ – m_x Oct 4 '13 at 17:32
4
\$\begingroup\$

As suggested by @m_x, This solution uses a parser of sorts, driven by the StringScanner built-in class. This is very compact, pretty readable, and gets rid of the state machine altogether:

Some methods to handle actions:

def accept(scanner)   
  @plain << scanner[1]
end

def reply_dont(scanner)
  @reply << IAC + DONT + scanner[1]
end

def reply_wont(scanner)
  @reply << IAC + WONT + scanner[1]
end

def ignore(scanner)
end

A list of telnet sequences:

# Telnet sequences to handle, and how to handle them

SEQUENCES = [
  [/#{IAC}(#{IAC})/, :accept],
  [/#{IAC}#{WILL}(.)/m, :reply_dont],
  [/#{IAC}#{WONT}(.)/m, :ignore],
  [/#{IAC}#{DO}(.)/m, :reply_wont],
  [/#{IAC}#{DONT}(.)/m, :ignore],
  [/#{IAC}#{IP}/, :ignore],
  [/#{IAC}#{DM}/, :ignore],
  [/(.)/m, :accept],
]

And the parser that uses them:

# Parse the the command.  Sets @plain and @reply

def parse_command(command)
  @plain = ''
  @reply = ''
  scanner = StringScanner.new(command)
  while !scanner.eos?
    SEQUENCES.each do |regexp, method|
      if scanner.scan(regexp)
        send method, scanner
        break
      end
    end
  end
end
|improve this answer|||||
\$\endgroup\$
  • \$\begingroup\$ Good job ! I think it's the best solution. I like the usage of the chain of responsibility pattern, it's extremely readable (each rule is explicit, and their precedence too), extendable, and easily testable. I'm impressed :) Just one more thing though : maybe the instance variables initialization should belong in #initialize. Oh, and sorry for not responding the chat invite, I was busy partying ^.^. Anyway, feel free to unaccept my answer and check this one, it's far better \$\endgroup\$ – m_x Oct 5 '13 at 15:50
  • \$\begingroup\$ This is what I ended up using in my code. I left the checkmark on @m_x's solution to reward him for giving me the idea. \$\endgroup\$ – Wayne Conrad Jul 11 '17 at 23:08
3
\$\begingroup\$

As suggested, I tried using a gem like state_machine. Well, not a gem like state_machine, but that particular gem. The state machine defintion is:

    state_machine :state, :initial => :idle do

      event :iac do
        transition :idle => :iac
        transition :iac => :idle
        transition [:will, :wont, :do, :dont] => :idle
      end

      event :will do
        transition :iac => :will
        transition :will => :idle
      end

      event :dm do
        transition :iac => :idle
      end

      event :ip do
        transition :iac => :idle
      end

      event :dont do
        transition :iac => :dont
      end

      event :other do
        transition all => :idle
      end

      event :do do
        transition :iac => :do
      end

      event :wont do
        transition :iac => :wont
      end

      before_transition :from => :do, :to => :idle, :do => :send_wont
      before_transition :from => :will, :to => :idle, :do => :send_dont
      before_transition :from => :idle, :to => :idle, :do => :accept_plain
      before_transition :from => :iac, :to => :idle, :on => :other, :do => :accept_unknown_iac
      before_transition :from => :iac, :to => :idle, :on => :iac, :do => :accept_plain

    end

The state_machine DSL is clean and relatively compact. Unfortunately, it is organized around events that cause transitions, rather than states that have transitions. That's not bad, but it's completely inside-out to how I think about state machines. It also makes you define actions separately from the transitions. The net result is that I can't see the state machine from this definition.

However, the state_machine gem has a rake task to generate a state diagram directly from the code. Here's what it generated for the above code:

state diagram

Other than the lack of actions, the state diagram generated by state_machine is very nice.

|improve this answer|||||
\$\endgroup\$
1
\$\begingroup\$

Here's a refactoring that extracts the definition of the state machine into a compact data structure. First a few lambdas for the actions:

    # State machine actions.
    #
    # @param plain [String] Accumulated plaintext
    # @param reply [String] Accumulated telnet reply
    # @param c [String] The current character

    EMIT_PLAIN = lambda do |plain, reply, c|
      plain << c
    end

    EMIT_PLAIN_UNKNOWN_SEQUENCE = lambda do |plain, reply, c|
      plain << IAC + c
    end

    EMIT_REPLY_DONT = lambda do |plain, reply, c|
      reply << IAC + DONT + c
    end

    EMIT_REPLY_WONT = lambda do |plain, reply, c|
      reply << IAC + WONT + c
    end

Then the state machine definition itself. Of all of the state-machine approaches I tried, this one has the most succinct definition:

    # The definition of the state machine used to recognize and handle
    # Telnet sequences.
    #
    # This is organized as nested hashes and arrays:
    #
    # The outer, or "state" hash is:
    # * key: state
    # * value: character hash
    #
    # The character hash is:
    # * key: character code, or :else
    # * value: action array
    #
    # The action array is a list of zero or more:
    # * lambda - The action to perform
    # * symbol - the next state

    STATES = {
      :idle => {
        IAC => [:iac],
        :else => [EMIT_PLAIN],
      },
      :iac => {
        IAC => [EMIT_PLAIN, :idle],
        WILL => [:will],
        WONT => [:wont],
        DO => [:do],
        DONT => [:dont],
        IP => [:idle],
        DM => [:idle],
        :else => [EMIT_PLAIN_UNKNOWN_SEQUENCE, :idle],
      },
      :will => {
        :else => [EMIT_REPLY_DONT, :idle],
      },
      :wont => {
        :else => [:idle],
      },
      :do => {
        :else => [EMIT_REPLY_WONT, :idle],
      },
      :dont => {
        :else => [:idle]
      }
    }

And, finally, the functions that drive the state machine:

    # Parse the the command.  Sets @plain and @reply
    #
    # @param command [String] The command to parse

    def parse_command(command)
      @plain = ''
      @reply = ''
      @state = :idle
      command.each_char do |c|
        character_hash = STATES[@state]
        raise "Unknown state #{@state.inspect}" unless character_hash
        actions = character_hash[c] || character_hash[:else]
        raise "Missing :else" unless actions
        apply_actions actions, c
      end
    end

    def apply_actions(actions, c)
      actions.each do |action|
        apply_action action, c
      end
    end

    def apply_action(action, c)
      if action.is_a?(Symbol)
        @state = action
      elsif action.respond_to?(:call)
        action.call @plain, @reply, c
      else
        raise "Unknown action #{action.inspect}"
      end
    end

This does a good job of showing the state machine in a compact and clear format, but the machinery that drives the state machine (parse_command and friends) seem like clutter. The state machine definition may be too compact--the amount of comments I felt necessary to describe it are a sign of that.

|improve this answer|||||
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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