後置記法のコードを添削してもらっちゃいました

rubyneko - Re: UK STUDIO 後置記法での計算 後置記法での計算の添削に便乗してみた - チナミニ

ujihisaさんとSixeightさんには感謝です。せっかくなんで添削してもらったコードを自分なりにちゃんと理解しときたいと思います。

ujihisaさんのコード

class String
  def rpn
    expr = self.split(" ")
    stack = []
    operators = %w[+ - * /]

    expr.each do |i|
      if operators.include? i
        stack = [calc(i, stack)]
      else
        stack < < i.to_i
      end
    end

    stack[0]
  end

  private
  def calc(operator, stack)
    stack.inject {|result, i|
      result.__send__(operator, i)
    }
  end
end

stackに文字列でなく数値を格納していく

確かに所々でto_i使い過ぎだ。これだけで全然スッキリするなぁ・・・

スコープは狭く

ですよねぇ。なんでクラス変数にしたんだろう、おれは。猛省。

Array.newを[]に

個人的にはなんかArray.newがしっくりくるんですよね。「Arrayをnewする」みたいな感じで。[]だとあまりそんな感じがしない。 まぁ慣れの問題な気もするので、[]も使っていこうと思います。

returnは省略

これはなんか個人的には解決してない問題でして。値を返すことを強調したいときはreturnをつけようとか思っていたんだけど。 ただ、Rubyは必ず値が返ってくることを考えると、やっぱり基本的に省略なのかなぁ。 ちなみにTwitterでぼそっとぼやいたらこんな反応が。

Array(...)を[...]に

[]で囲むのでもいいのか。知らなかった。

calcがArrayを返すより、stackに入れるときにArrayにするべし

説明がなんかHaskellerっぽいですな。 calc::char->[int]->[int]ってどう言う意味だっけ。確か、第1引数の型->第2引数の型->返り値の型だったかな。[int]はintの配列ってことかな・・・ 確かにcalcの処理内容を考えると[int]よりintの方がわかりやすいな。ふむ。

eachのブロックを{}からdo endに

そもそも、自分のコーディング規約を守ってない件。複数行のブロックはdo...end使うって決めたのに! 副作用を目的としているケースでは、こちらのが直感的とあるけど、何故だかちょっとわからない。直感的ってのもまた人それぞれな部分あるしなぁ。

if i == "+" || i == "-" || i == "*" || i == "/"をArray#include?を使ってシンプルにまとめた

このifは自分でも汚いなとは思ってた。各演算子を配列にまとめてinclude?で判断か。ブロック引数iの値が配列に含まれていればtrueを返すから、この場合だと"+", "-", "*", "/"のどれかだとtrueになると。

calcについて

result.__send__(operator, i)
って記述を見たとき、「あれ、resultに代入しなくていいの?」とか思ったんだけど、ちゃんと調べてみると途中のブロックの返り値は次の呼び出しの時にresultに渡されるんだとさ。ちゃんと調べろよって話ですね。 ujihisaさんにはさらに短かくしたコードも書いてもらってるんだけどまだちょっと追えてない。これはまた後日こっそり調べとく。

Sixeightさんのコード

関係ないけどSixeightさんってtyoroの後輩だっけ?
class String
  # Reverse Polish Notation
  # '4 5 +'.rpn => 4 + 5 = 9
  def rpn
    stack = []
    opr = %q[ + - * / ]

    split(' ').each do |i|
      if opr.include? i
        stack[-1] = stack[-2].__send__(i, stack.pop)
      else
        stack < < i.to_i
      end
    end
    stack.first
  end
end
一応、最終的なコードを見せてもらう。(もちろん、その前のコードも読みましたよ)全体的にSixeightさんもujihisaさんのコードを見たあとのせいか、基本的には上で書いたことと同じ感じかなー。 ただ、演算の処理をメソッドで別にせず1行で済ませてますな。あとオレの場合はめんどくさくて、stackを全部上書きしちゃってるけど、Sixeightさんはpopで取り出してちゃんと処理してる。 個人的にはstack[-1]とstack[-2]が気になるかな。インデックスの-が個人的に好きじゃないようで。まぁあんなcalcメソッド書いといて何言ってるんだって感じですよね。すみません。 逆ポーランド記法の仕様についてはあまりちゃんと調べてなかったり。なんかSchemeの前置記法と似た感覚で複数の値も計算するもんだと思ってたけど違うのかな。

追記

はてブにSixeightさんのコメントが!
インデックスの-は僕も否定派なんですが、スタックから取り出す順番をひっくり返すのが思い浮かばなくて><精進しなきゃ
やっぱ-は否定的なのかー。お互いがんばりまっしょい。

書き直したコード

class String
  def rpn
    expr = self.split(" ")
    stack = []
    operators = %w[+ - * /]

    expr.each do |i|
      if operators.include? i
        stack = [stack.inject{|result, item| result.__send__(i, item)}]
      else
        stack < < i.to_i
      end
    end

    stack.first
  end
end
ujihisaさんのコードをほとんど反映させた感じ。例外処理とか考えてないよ。あとスタックにある数値も全部計算する仕様のまま。