Did not pass the interview about an RPN Calculator

Question

I recently received a simple interview assignment as to implement an RPN calculator in Ruby. The program takes user input and quits when the user presses 'q'.

I did it in under 30 minutes and was quite confident about my solution, but the interviewers did not like it. I would love to know the flaws it has so I can avoid being overconfident next time and learn better coding practices!

#!usr/bin/env ruby

# A Ruby RPN Calculator 
#Note: decimal numbers are rounded to nearest tenth
=begin
example input/ouput
> 2
2
> 9
9
>3
3
> +
12
> * 24
=end

class RPNCalculator

  attr_reader :operators
  def initialize(operators)
    @operators=operators
  end

  def start
    print '> '
    n = gets.strip
    stack = []

    while n!= 'q'
      if n =~ /\d/
        stack.push(n.to_f)
        puts n
      elsif self.operators.include? n
        operands = stack.pop(2)
        res = operands.reduce(n.to_sym)
        (res.to_i == res) ? (puts res.to_i) : (puts "%0.1f" % res)
        stack.push(res)
      else 
        puts "Invalid input"
      end
      print '> '
      n = gets.strip
    end
  end
end

rpn = RPNCalculator.new(["+","-","*","/"])
rpn.start

Answer 1

I don't know how your assignment looked exactly, so some of the below may be misguided criticisms. I've just tried to poke as many holes in the code as possible; fair or otherwise. Something will probably overlap with your interviewer's thoughts. But of course, I don't know what that might be, so there's no real order to these points - I just wrote them as I thought of them.

• Separate the logic from the REPL user interface. It'd be nicer if you could push input to the calculator from anywhere. And it'd be much easier to test the code.

• Speaking of: Tests would be nice to see. If the place you interviewed is really big on TDD/BDD, maybe that's what they wanted to see more than anything. Interview assignments (when done well) are as much about the candidate's process as it is the result.

• #start is a rather long method and it pretty much does, well, everything. If you call it, it starts a loop you won't be able to exit from code. That's kinda scary. The method just does too much.
  Again, a different structure will help. Also, Ruby's case statements are pretty powerful. Might come in handy instead of if...elsif...else.

• The regex used to match numbers is very loose. It'll accept anything as long as there's a digit somewhere in it. So "u wot m8" is technically a number in the eyes of your code.

• The (res.to_i == res) ? (puts res.to_i) : (puts "%0.1f" % res) line is several kinds of nasty. It'd be much cleaner if you put the puts first, and used the ternary for its arguments instead. Right now it's "if A then puts, otherwise if B then... well, then also puts". Also you don't want to use a ternary's branches for anything except the simplest things. You'll want to use them to decide between values, not actions. So calling to_i on stuff or doing simple arithmetic is fine, but don't go too far. If you want to do more, use a proper if..else.
  Something like this would make more sense:

  puts res.to_i == res ? res : "%0.1f" % res
  

  Or you could use %g instead, which'll ignore decimals for integers:

  puts "%g" % res.round(1)
  

  However, it'll truncate large numbers so it's not a clear winner.

• Your code accepts a list of operators, but it assumes:

  1. that any string you pass is actually valid (otherwise the reduce call will break)
  2. that all the operators have an arity of 2.

  So the code is halfway configurable, and halfway not. Suppose you want to support a ! operator for factorial: For one, ! is not a method on Float, so reduce(n.to_sym) will fail. For another, the factorial operator only has one operand, not two.
  Frankly, I wouldn't bother making this configurable unless I was asked to. Or I'd ask if I was unsure, rather than make assumptions.

• Speaking of operator arity: The script crashes if the stack's empty and you enter an operator. You assumed that all operators would always have two operands, but you also assumed that there always are two operands.

• Not sure what the last example input (* 24) is supposed to do. Push 24 to the stack and multiply? Complain about the input? (Edit: As pointed out in the comments, it's probably just missing linebreak. That would make sense. Even though I should've figured it out myself, I'll just turn that around and say you should proof-read your code comments and documentation :)
  Regardless of its intended meaning, it's interesting as test input: It prints * 24 back to you, because you print the raw input, rather than the parsed value. So you don't know what the script actually made of your input. What it did, though, was push 0.0 to the stack, because the code thinks it's a number, but "* 24".to_f just returns zero.

• Speaking of what you print, echoing the user's input right back is sort of pointless. I mean, the user just wrote it. Printing the stack (or at least the input after interpretation) would likely be more useful.

• Naming. The variable n is too short to mean much. I suppose it means "number", but that only makes sense if the input is indeed a number. I'd just call it input. Also, res is unnecessarily shortened. It's not that bad to write result instead. There's really no reason to abbreviate.
  Otherwise, naming is fine - I like that operators and operands are both full words. Still, as mentioned, #start is not a great name for a method that does everything including reading, evaluating, printing and (ironically) stopping. But that's less a question of changing the name; the method itself should be refactored.

• If you want to be fancy, you could make a Stack class instead, and implement #*, #+, etc. as methods on that class.

• Small fry, but ["+","-","*","/"] could be written more simply as %w(+ - * /). But again, I'd skip making it configurable.

Now, as mentioned I don't know what your exact exercise was. You built a REPL calculator, but perhaps they wanted something that could parse a full expression like "3 4 * 2 /" => 6? Or a calculator "engine" that's callable from code? If that's the case, you kinda went the wrong way to begin with :)

Edit: I should add that I don't intend for this to be harsh. As 200_success points out, it's not at all bad for < 30 mins of coding while under pressure.

Answer 2

This isn't bad for interview code written under time constraints and mental pressure.

My first observation is that the object design is deficient. There isn't much point to defining an object with one method. You could just as well have written this as a function. If you were to write RPNCalculator as an object, then I would expect the stack to be a property of the calculator, not of the run loop.

The behaviour is somewhat odd on stack underflow. I would expect it to print some kind of error message and leave the stack unchanged. Your code does leave the stack unchanged (but for a curious reason). If you give + as the first input, then it crashes with in `%': can't convert nil into Float (TypeError).

The code is designed do accommodate only operators that take two operands and return one number. You cannot have a cos operator, for example. Also, a useful RPN calculator would support stack-manipulation operations such as clear, drop, dup, roll, rolld, and swap.

For clearer and less redundant code, the prompting code should be extracted out of the loop. I would write…

def start
  while (input = prompt('> ')) != 'q' do
    …
  end
end

private
def prompt(s)
  print s
  gets.strip
end

The ternary expression (res.to_i == res) ? (puts res.to_i) : (puts "%0.1f" % res) is really an if-else. It would be better written as puts (res.to_i == res) ? res.to_i : "%f" % res. (In my opinion, one decimal place is insufficient — you can't even do decent dollars-and-cents calculations.)

The shebang line is not quite right: It should be #!/usr/bin/env ruby.