RPN calculator with interactive and non-interactive modes

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)); e

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 ;)

StackExchange Code Review Q#91235, answer score: 4