8
\$\begingroup\$

While writing this review, I edited the code until it became something quite different from the original. In addition to the issues I mentioned, I ended up adding features:

  • More operators / commands, including trigonometry in degree and radian modes.
  • Support for multiple commands on a line, separated by whitespace. Each line is transactional: if any part fails, the stack remains unchanged.
  • A rudimentary help system, while maintaining a spartan interface.
  • Readline support with tab completion when running in a TTY, and a quiet mode for accepting input from a pipe.

Concerns:

  • I overrode Array#dup to do something completely different.
  • In interactive mode, I want all output on stdout. In non-interactive mode, I want only the real output on stdout, and only critical error messages on stderr. The switching mechanism for the two modes seems cumbersome.
require 'readline'

# Mixin for an Array
module RPNOperators
  def +; push(pop + pop); end
  def -; push(-pop + pop); end
  def *; push(pop * pop); end
  def /; push(1.0 / pop * pop); end
  def ^; x = pop; push(pop ** x); end

  def deg(quiet=false)
    @conv = Math::PI / 180
    info 'In degree mode. (Use the "rad" command to switch to radian mode)' unless quiet
  end

  def rad(quiet=false)
    @conv = 1.0
    info 'In radian mode. (Use the "deg" command to switch to degree mode)' unless quiet
  end

  def pi;       push(Math::PI); end
  def cos;      push(Math.cos(pop * @conv)); end
  def sin;      push(Math.sin(pop * @conv)); end
  def tan;      push(Math.tan(pop * @conv)); end
  def acos;     push(Math.acos(pop) / @conv); end
  def asin;     push(Math.asin(pop) / @conv); end
  def atan;     push(Math.atan(pop) / @conv); end

  def e;        push(Math::E); end
  def chs;      push(-pop); end
  def inv;      push(1.0 / pop); end
  def cbrt;     push(Math::cbrt(pop)); end
  def sqrt;     push(Math::sqrt(pop)); end
  def ln;       push(Math::log(pop)); end
  def log;      push(Math::log10(pop)); end

  def clear;    super; end
  def drop;     pop; end
  def dup;      x = pop; push(x); push(x); end
  def roll;     unshift(pop); end
  def rolld;    push(shift); end
  def swap;     x = pop; y = pop; push(x); push(y); end

  def quit
    throw :quit
  end

  def help(stream=$stdout)
    stream.puts "Available commands:"
    stream.puts RPNOperators.public_instance_methods.join(' ')
    stream.puts
    if deg? then deg else rad end
  end

  private
  def deg?;     @conv != 1.0; end
  def rad?;     @conv == 1.0; end

  def info(s)
    puts s
  end
end

class RPNCalculator
  class InvalidCommand < Exception; end
  class StackUnderflowError < Exception; end

  class Stack < Array
    include RPNOperators

    def initialize(*args)
      super
      rad(true)
    end

    def pop(*n)
      raise StackUnderflowError.new if (n[0] || 1) > size
      super
    end

    def shift(*n)
      raise StackUnderflowError.new if (n[0] || 1) > size
      super
    end
  end

  def initialize
    @stack = Stack.new
    Readline.completion_proc = proc do |s|
      available_commands.grep(/^#{Regexp.escape(s)}/)
    end
  end

  def run
    catch(:quit) do
      loop do
        case input = prompt('> ')
        when nil
          throw :quit
        when /\s/
          command(*input.split(/\s+/))
        else
          command(input)
        end
      end
    end
    raise @error if @error
  end

  def command(*cmds)
    error_stream = $stdin.tty? ? $stdout : $stderr
    last_command = nil
    begin
      transaction do
        cmds.each do |cmd|
          last_command = cmd
          case cmd
          when /\A[+-]?\d*\.?\d+\Z/
            @stack.push(cmd.to_f)
          else
            if available_commands.include?(cmd)
              @stack.send(cmd.to_sym)
            else
              raise InvalidCommand.new
            end
          end
        end
      end
    rescue InvalidCommand
      error_stream.puts "Invalid input: \"#{last_command}\""
      @stack.help(error_stream)
      error_stream.puts
      error_stream.puts "Stack (top to bottom):"
    rescue StackUnderflowError
      error_stream.puts "#{last_command}: not enough operands"
    rescue Exception => e
      error_stream.puts e
    end
    display
  end

  def available_commands
    @available_cmds ||= RPNOperators.public_instance_methods(false).map { |sym| sym.to_s }
  end

  private
  def prompt(s)
    input = $stdin.tty? ? Readline.readline(s, true) : gets
    input.nil? ? nil : input.strip
  end

  def display
    puts @stack.reverse.map { |n| '%-8g' % n }.join('    ')
  end

  def transaction
    begin
      # Can't use @stack.dup, which has been overridden to do something else!
      saved_stack = @stack.clone
      @error = nil
      yield
    rescue Exception => e
      @error = e
      @stack = saved_stack
      raise
    end
  end
end

begin
  RPNCalculator.new.run
rescue Exception
  exit(1)
end
\$\endgroup\$
4
\$\begingroup\$

Neat! I'm digging the dual TTY/pipe support, although it does introduce some complexity. My gut feeling is that I'd probably prefer a calculator object that is agnostic about the source of its input and the destination of its output, but which can be subclassed as necessary.

But you've chosen this direction, and I'm not saying the alternative would necessarily be nicer, so I'll take it as given, and focus on the internals.

  • Between RPNOperators and Stack's own methods do you even need to subclass Array? Or would it be neater to simply let Stack be a plain object that wraps an array? I'm only asking because subclassing array means Stack inherits a lot of methods, not all of which are necessarily desirable.

    But it's your call; the argument can be made either way. Personally, I just prefer specialized APIs and object composition, to "crowded" APIs caused by subclassing very generic classes like Array. But it's as much opinion as anything.

    A compromise might be to wrap the array, and use Forwardable to delegate relevant methods to it with minimal hassle. No need to duplicate the Array methods that you do want.

    In any event, not subclassing Array would of course avoid (most) violations of the Liskov principle. You'd still be overriding #dup from Object, but you'd avoid overriding #+, #-, and #* from Array.

  • Stack#pop and Stack#shift
    Instead of making them variadic, it'd be nicer to define the parameter of each with a default value (n = 1), since that's what appears to be intended. And the super methods aren't variadic either.
    Edit: I was wrong here. As pointed out in the comments, passing an argument to Array#pop causes it to always return an array, even if n = 1. So Array#pop is actually 0-1 variadic, but the closest you can get is to make a 0-n variadic. So, with that in mind, I'll instead recommend n.first instead of n[0] - just to still recommend something :)
    Also, note that super will still complain if more than 1 argument is passed.

  • Readline.completion_proc
    You could do available_commands.select { |cmd| cmd.start_with?(s) } - seems more straightforward to me than interpolating a regex with an escaped string.

  • #command
    This is a rather large method. A lot of is of course the exception handling, but it's also got a lot of nesting.

    One thing you might do right away is to pull the if..else out of the case statement's own else branch:

    case cmd
    when /\A[+-]?\d*\.?\d+\Z/ # Use a named constant instead?
      @stack.push(cmd.to_f)
    when *available_commands  # splat!
      @stack.send(cmd.to_sym)
    else
      raise InvalidCommand.new
    end
    

    I'd also suggest making your custom exception classes take a constructor argument (or several), so you can pass the invalid command and possibly other pieces of state along with the exception, rather than rely on the local last_command variable. (You could also be cheeky and do raise cmd, and then do rescue RuntimeError => failed_command, but that's pretty hacky.)

    You can also avoid a level of nesting since rescue statements don't need to be in an explicit begin...end block, and you have ensure:

    def command(*cmds)
      # ...
    rescue InvalidCommand
      # ...
    rescue StackUnderflowError
      # ...
    rescue Exception => e
      # ...
    ensure
      display
    end
    

    Lastly, and this is just because I like short methods, you could extract the "meat" (the case statement) into a private #eval_command (or something) method, so the transaction block becomes just:

    transaction do
      cmds.each(&method(:eval_command))
    end
    
  • Exception classes
    Just a minor naming thing, but I'd either drop the -Error suffix from StackUnderflowError or add it to InvalidCommand, just to keep them consistent.

  • stdout/stderr
    Unless I'm mistaken, $stdin.tty? isn't likely to change during the lifetime of an RPNCalculator object. So you could set the output and error streams as instance variables in the constructor.

    You could also add a (private) #warn method which would shadow the global Object#warn, writing to either stderr or stdout as necessary. It would let you call the more "symmetrical" methods puts and warn, instead of puts and error_stream.puts.

  • #available_commands
    Tiny, tiny thing, but .map { |sym| sym.to_s } can be just .map(&:to_s).

  • Overriding #dup
    As mentioned, you can't completely avoid overriding a #dup method because your class will always inherit it from Object. Though I suppose you call the command something other than dup and solve the problem that way ;)

\$\endgroup\$
  • \$\begingroup\$ Part of my thinking for this Stack design is for RPNOperators to get stack operations like push, pop, shift, unshift, and clear in a compact way. \$\endgroup\$ – 200_success May 20 '15 at 18:59
  • 1
    \$\begingroup\$ [1337].pop(1) doesn't behave the same as [1337].pop. I was trying to preserve that behaviour. \$\endgroup\$ – 200_success May 20 '15 at 19:03
  • \$\begingroup\$ @200_success re Stack < Array: I figured as much, and it does make a lot of sense. As mentioned, the wrap-vs-subclass it's as much about my personal opinion as anything. Re pop: You're right - my mistake. I'll edit. \$\endgroup\$ – Flambino May 20 '15 at 19:05

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.