class RPNCalculator
def evaluate rpn
array = rpn.split(" ").inject([]) do |array, i|
if i =~ /\d+/
array << i.to_i
else
b = array.pop(2)
array << b[0].send(i, b[1])
end
end
p array.pop
end
end
Refactoring feedback for Reverse Polish Notation (RPN) or Postfix Notation
-
02-06-2022 - |
Frage
One of the pre-work exercises for Dev Bootcamp is an RPN calculator. I made it work but would like refactoring feedback. Any and all help to make this code cleaner is greatly appreciated.
class RPNCalculator
def evaluate(rpn)
a = rpn.split(' ')
array = a.inject([]) do |array, i|
if i =~ /\d+/
array << i.to_i
else
b = array.pop(2)
case
when i == "+" then array << b[0] + b[1]
when i == '-' then array << b[0] - b[1]
when i == '*' then array << b[0] * b[1]
when i == '/' then array << b[0] / b[1]
end
end
end
p array.pop
end
end
calc = RPNCalculator.new
calc.evaluate('1 2 +') # => 3
calc.evaluate('2 5 *') # => 10
calc.evaluate('50 20 -') # => 30
calc.evaluate('70 10 4 + 5 * -') # => 0
Lösung
Andere Tipps
I tend to prefer avoiding case..when
in favor of lookup tables. So I'd change your code to:
class RPNCalculator
def evaluate(rpn)
a = rpn.split(' ')
array = a.inject([]) do |array, i|
if i =~ /\d+/
array << i.to_i
else
array << array.pop(2).reduce(op(i))
end
end
p array.pop
end
private
def op(char)
{'+'=>:+, '-'=>:-, '/'=>:/, '*'=>:*}[char]
end
end
I also don't believe you should only be popping off 2 operands. "1 2 3 +" would be valid RPN, evaluating to 6. The entire stack should be reduced. This also avoids the mutation, which is a good thing, as it follows a more functional style.
class RPNCalculator
def evaluate(rpn)
a = rpn.split(' ')
array = a.inject([]) do |array, i|
if i =~ /\d+/
[*array, i.to_i]
else
[array.reduce(op(i))]
end
end
p array.pop
end
private
def op(char)
{'+'=>:+, '-'=>:-, '/'=>:/, '*'=>:*}[char]
end
end
I removed the other mutation here too, by using [*arr, value]
instead of actually modifying the array.
Finally, I'd avoid printing directly from your #evaluate
method and just return the number. I'd also (again) avoid the mutation:
class RPNCalculator
def evaluate(rpn)
a = rpn.split(' ')
stack = a.inject([]) do |stack, i|
if i =~ /\d+/
[*stack, i.to_i]
else
[stack.reduce(op(i))]
end
end
stack.last
end
private
def op(char)
{'+'=>:+, '-'=>:-, '/'=>:/, '*'=>:*}[char]
end
end
I renamed 'array' to 'stack', since it is a parser stack and is less generic than just array
.