【Ruby】リファクタリング。まじめに。

せかいさんのコードは綺麗じゃないね
アルゴリズムの選択も筋がいいとはあまり思いませんし
変数名とかメソッド名とか他人に読ませるようのコードじゃないね
tok とか _findとか全然意図が伝わない。
せかいさんのリファクタリングしたコードがサイトに乗ってますが
正直、全然綺麗になってるとは感じません。
可読性を重視してコード書いてみてください

という師匠からのメールを元にこのコードをさらに見直すよ。
【Ruby】【アルゴリズム】宣教師と人食い問題（全列挙型Ver） ※リファクタリング後 - せかいや

この解法でのアルゴリズムの選択は間違えてるけど
これは話の流れから全列挙型になっているだけであって、
たぶん師匠もそれは知っているから、その所以外にも指摘があるってことだと思う。

順に見ていこう。
　

だめなところ

def make_candidate

def make_approvals

両方とも複数を返しているのに、
命名規則が単数・複数とバラバラ。

あと、make_も必要ないか。
　

#起こりうる可能性を全て列挙（2**6通り）
def make_candidate
  results = ["000000"] #人人人鬼鬼鬼が左岸
  6.times do |i|
    _results = results.dup
    _results.each do |res|
      tmp2 = res.dup
      tmp2[i] = "1"
      results << tmp2
    end
  end
  results
end

candidates メソッドでは「candidates」という概念を返すのだから、
resultsという変数名はやめよう。candidatesにしよう。

と思ったけど、そうすると変数名とメソッド名が一緒になる。
変数名にはアンスコをつけよう。

def candidates
  _candidates = ["000000"] #人人人鬼鬼鬼が左岸
  6.times do |i|
    _results = _candidates.dup
    _results.each do |res|
      tmp2 = res.dup
      tmp2[i] = "1"
      _candidates << tmp2
    end
  end
  _candidates
end

うーん。。。
変数名が　
_candidates
_results
res
tmp2
と、ますますカオスなことになった。

ここだけで考えるのではなくて、
この2メソッドは統合しよう。

その上で変数名を考えてみよう。

#起こりうる可能性を全て列挙（2**6通り）
def candidates
  _candidates = ["000000"] #人人人鬼鬼鬼が左岸
  6.times do |i|
    _results = _candidates.dup
    _results.each do |res|
      tmp2 = res.dup
      tmp2[i] = "1"
      _candidates << tmp2
    end
  end
  _candidates
end

#食べられないケースだけ抽出
def approvals(results)
  apps = []
  results.each do |res|
    data = res.split("").map{|a|a.to_i}
    a = data[0,3].inject(:+)
    b = data[3,3].inject(:+)
    #全員が左or右 もしくは 左岸がOK&&右岸がOK
    apps << res if (a==3 || a==0) || ((3-a) >= (3-b) && a>=b)
  end
  apps
end

　
と思ったけれども、やっぱりここは分けておこう。
全列挙を行って「食べられないケース」だけ抽出するのは、
1度のループでやろうとするとちょっと複雑になる。

こうかな。

#起こりうる可能性を全て列挙（2**6通り）
def candidates
  _candidates = ["000000"] #人人人鬼鬼鬼が左岸
  6.times do |i|
    copy = _candidates.dup
    copy.each do |x|
      tmp = x.dup
      tmp[i] = "1"
      _candidates << tmp
    end
  end
  _candidates
end

　
でも_candidatesは、プライベートメソッドみたいに見える。
やっぱりここはresultにしよう。

def candidates
  result = ["000000"] #人人人鬼鬼鬼が左岸
  6.times do |i|
    copy = result.dup
    copy.each do |x|
      tmp = x.dup
      tmp[i] = "1"
      result << tmp
    end
  end
  result
end

ちょっとは良くなった。

こういう感じで直してみよう。

そういえば前に師匠が
リファクタリング前コードと
リファクタリング後コードを分けてメールしてくれたことがあった。

あのメールを見返すと発見があるかもしれない。
みてみよう。