Pulling indexes from an array and variable scope

Question

I'm running through TestFirstRuby and I've been stuck on problem 12, building a Reverse Polish Notation calculator. I've gotten through all of the tests except for the last one, asking me to take a string ("1 2 3 * +" and then "1 2 3 * + 4 5 / -"), and then evaluate the expression.

What I'm trying to do is convert the string to an array, changing the numbers into integers and the operators into symbols, then go through the array and evaluate the expression anytime it comes to an operator.

Here's the relevant part of the code:

  def initialize
    @expression = ''
  end

  def tokens(string)
    tokens = string.split(' ')
    tokens.map! { |digit| /[0-9]/.match(digit) ? digit.to_i : digit.to_sym }
  end

  def evaluate(string)
    #1 2 3 * +
    #1 2 3 * + 4 5 - /
    total = 0
    @expression = tokens(string)
    @expression.map!{|item|
      index = @expression.index(item)
      if item == :+
        total = total + (@expression[index-1] + @expression[index-2])
        2.times {@expression.delete_at(index-1)}
      elsif item == :-
        total = total + (@expression[index-1] - @expression[index-2])
        2.times {@expression.delete_at(index-1)}
      elsif item == :x
        total = total + (@expression[index-1] * @expression[index-2])
        2.times {@expression.delete_at(index-1)}
      elsif item == :/
        total = total + (@expression[index-1].to_f / @expression[index-2])
        2.times {@expression.delete_at(index-1)}
      end
    }
    total
  end

What I WANT to happen is this: for each item in the array, it checks if it matches any of the symbols. If there's a match, it changes the element two spaces back from the symbol into the value of whatever the expression is (so 2 3 * becomes 5 3 *). Then, I"m trying to delete the operator and the integer immediately before it, leaving only the evaluated value. I'm doing this by running delete_at twice on the index just before the operator (ideally, 5 3 * goes to 5 * and then just 5). Then it'll move on to the next element in the array.

What I THINK is going wrong, and am having trouble fixing: I think something's going on with the variable scope here. I'm trying to get the expression to be permanently changed every time it runs the code on whatever element it's currently on in the each loop. For each element, a variable 'index' is set using @expression.index(item). This should reset for each element in the each loop. I THINK what's going on is that the original @expression array is being called on for each iteration of the each loop, unchanged from each iteration of the each loop.

The error I'm getting is saying that when it gets to the '+' at the end of the first test string ('1 2 3 * +'), it's trying to add using :x, meaning that when it's calling for the two variables to add together (@expression[index-1] + @expression[index-2]), it's pulling the symbol, which I thought should have been deleted from @expression already. So what I'm hoping will evaluate as 6 + 1 is being evaluated as 3 + :x, which wouldn't work. It's pulling elements from the original array, instead of pulling from the array as it's changed.

Hopefully I'm explaining this clearly enough. Any advice would be great. I'm thinking there's something with scope going on, but I can't find anything specific to this kind of problem to help me out. I've tried different ways of coding this (.map, .each_with_index, .map.with_index, and others), and I'm getting the same problem each time.

Answer 1

You have a tremendous amount of redundant code. In particular, you replicate the operations for each of the four operators. Here is a more Ruby-like way of implementing your calculator.

Code

def evaluate(string)
  arr = create_arr(string)
  until arr.size == 1
    i = arr.index(arr.find { |e| e.is_a? Symbol })
    arr[i-2] = arr[i-2].send(arr[i], arr[i-1])
    arr.delete_at(i)
    arr.delete_at(i-1)
  end
  arr.first
end

def create_arr(string)
  string.split(/\s+/).map { |e| e =~ /-?[0-9]+/ ? e.to_i : e.to_sym }
end

The line in create_arr could alternatively end : e } (sent accepts a string or symbol for the method), in which case e.is_a? Symbol would be changed to e.is_a? String.

Examples

evaluate("3 4 * 2 / 3 - 2 *")     #=>  6
evaluate("10    2 / 3 + 2 / 2 -") #=>  2
evaluate("8 -2 / 1 +")            #=> -3
evaluate("5 1 2 + 4 * + 3 -")     #=> 14

Explanation

Suppose

string = "2 3 4 * 2 / +"

Step 1

arr = create_arr(string)              #=> [2, 3, 4, :*, 2, :/, :+]
arr.size == 1                         #=> false
  v = arr.find { |e| e.is_a? Symbol } #=> :*
  i = arr.index(v)                    #=> 3
  arr[i-2] = arr[i-2].send(arr[i], arr[i-1])
    # arr[1] = arr[1].send(arr[3], arr[2])
    # arr[1] = 3.send(:*, 4)          #=> 12
  arr                                 #=> [2, 12, 4, :*, 2, :/, :+]
  arr.delete_at(i)                    #=> :*
  arr                                 #=> [2, 12, 4, 2, :/, :+]
  arr.delete_at(i-1)                  #=> 4
  arr                                 #=> [2, 12, 2, :/, :+]

Step 2

arr.size == 1                         #=> false
  v = arr.find { |e| e.is_a? Symbol } #=> :/
  i = arr.index(v)                    #=> 3
  arr[i-2] = arr[i-2].send(arr[i], arr[i-1])
    # arr[1] = arr[1].send(arr[3], arr[2])
    # arr[1] = 12.send(:/, 2)         #=> 6
  arr                                 #=> [2, 6, 2, :/, :+]
  arr.delete_at(i)                    #=> :/
  arr                                 #=> [2, 6, 2, :+]
  arr.delete_at(i-1)                  #=> 2
  arr                                 #=> [2, 6, :+]

Step 3

arr.size == 1                         #=> false
  v = arr.find { |e| e.is_a? Symbol } #=> :+
  i = arr.index(v)                    #=> 2
  arr[i-2] = arr[i-2].send(arr[i], arr[i-1])
    # arr[0] = arr[0].send(arr[2], arr[1])
    # arr[0] = 2.send(:+, 6)          #=> 8
  arr                                 #=> [8, 6, :+]
  arr.delete_at(i)                    #=> :+
  arr                                 #=> [8, 6]
  arr.delete_at(i-1)                  #=> 6
  arr                                 #=> [8]

Step 4

arr.size == 1                         #=> true
arr.first                             #=> 8