【プログラミング】リファクタリング。（状態の管理の統一。情報の隠蔽化。インスタンス変数の使いどころ）

　
これでリファクタリング完了！って師匠にメールしたら、返事が来たよ。

変数名の選択はよくなったと思いますよ。
でも、まだ十分じゃないと思います。

まず状態の管理が文字列やったり数値やったりで
よくわからないです

@history.length % 2 == 0
ってなってるけど、これどういう意味かわからない。

さらに言えば、000000の前半が人間で後半が非人間っていう情報を
複数箇所で使ってるよね
そんなにその情報に依存するなら、隠蔽化したほうがいいと思うよ

あと、インスタンス変数って必要ですかね
素直に引数に渡すほうがいいような気がしますが。
@patternsってインスタンス変数？定数なんやないすかね

確かに。。
一個ずつみていこう。

その１。状態の管理をStringで統一

状態の管理が文字列やったり数値やったりで
よくわからないです

確かに。
でも文字列で持たせないと
"000000"　が0になって扱いにくいし、
文字列のままだと「人間が右岸に合計何人いるのか」が計算しづらい。
この計算ね↓

 previous = previous_string.split("").map{|a|a.to_i}
 p1 = previous[0,3].inject(:+)

　
そうか！合計数を求めるのは足し算ではなくて
String#countを使えばいいんだ！

 p1 = previous[0,3].count("1")

こう書けばいいんだ。

■改善前

def dejavu?(state)
  @history.each_with_index do |previous_string, i|
    previous = previous_string.split("").map{|a|a.to_i}
    p1 = previous[0,3].inject(:+)
    p2 = previous[3,3].inject(:+)
    s1 = state[0,3].inject(:+)
    s2 = state[3,3].inject(:+)
    return true if (p1 == s1) && (p2 == s2) && (i%2 == (@history.length)%2)
  end
  false
end

■改善後

def dejavu?(state, history)
  history.each_with_index do |previous, i|
    p1 = previous[0,3].count("1")
    p2 = previous[3,3].count("1")
    s1 = state[0,3].count("1")
    s2 = state[3,3].count("1")
    return true if (p1 == s1) && (p2 == s2) && (i%2 == (history.length)%2)
  end
  false
end

良くなった！

その２

@history.length % 2 == 0
ってなってるけど、これどういう意味かわからない。

うーん、、

if history.length%2 == 0 #右から左へ

ってコメント書いてるからなあ。。
メソッドにして切り出すことも出来るけど大げさになりすぎるので、ここはこのままで。

その３。情報を隠蔽化

000000の前半が人間で後半が非人間っていう情報を
複数箇所で使ってるよね
そんなにその情報に依存するなら、隠蔽化したほうがいいと思うよ

　
たしかに。
メソッド化しよう。

■改善前

p1 = previous[0,3].inject(:+)
p2 = previous[3,3].inject(:+)
s1 = state[0,3].inject(:+)
s2 = state[3,3].inject(:+)

■改善後

previous_m = missionaries_state(previous).count("1")
previous_c = cannibals_state(previous).count("1")
state_m = missionaries_state(state).count("1")
state_c = cannibals_state(state).count("1")

変数名も、可読性向上のために変更。

その４。トップレベルでインスタンス変数は避ける

あと、インスタンス変数って必要ですかね
素直に引数に渡すほうがいいような気がしますが。
@patternsってインスタンス変数？定数なんやないすかね

　
なるほど。。

インスタンス変数にするか否かは、
複数のメソッドから参照するという点で、
インスタンス変数にするほうが見やすくなる（引数が減ることも含めて）と考えたので、インスタンス変数化したのですが。
引数に渡したほうが素直かなあ。。

気持ちは分かりますが、それってつまりはグローバル変数と同じでしょ？
グローバル変数が更新されていくのって、あまり良くはないです。

なるほど。
変数のスコープを安易に広げるのはよくない。
それに、明示的に作ったクラスのインスタンス変数ならわかるけど、トップレベル（Objectクラスの特異クラス）のインスタンス変数、というのはあまり行儀が良くない。

引数が減って綺麗に見えるけど、インスタンス変数の使いどころは考えないと。

修正後コード全量

def cases
  answers = ["000000"] #宣宣宣食食食が左岸
  6.times do |i|
    copy = answers.dup
    copy.each do |state|
      new_state = state.dup
      new_state[i] = "1"
      answers << new_state
    end
  end
  answers
end

def missionaries_state(state)
  state[0,3]
end
def cannibals_state(state)
  state[3,3]
end

#食べられないケースだけ抽出
def sieve(cases)
  answers = []
  cases.each do |caze|
    a = missionaries_state(caze).count("1")
    b = cannibals_state(caze).count("1")
    #全宣教師がどちらかの端に存在 もしくは 左岸&&右岸がOK
    answers << caze if (a==3 || a==0) || ((3-a)>=(3-b) && a>=b)
  end
  answers
end

def possible?(state, next_state, history)
  return false if state == next_state
  total_mover = 0
  if history.length%2 == 0 #右から左へ
    state.split("").each_with_index do |n, i|
      if n == "0"
        return false if next_state[i] != "0" #0->1（右岸に移動）は出来ない
      else
        total_mover += 1 if next_state[i] != "1"
      end
    end
    return false if total_mover > 2 #ボートの定員は2名
  else #左から右へ
    state.split("").each_with_index do |n, i|
      if n == "1"
        return false if next_state[i] != "1" #1->0（左岸に移動）は出来ない
      else
        total_mover += 1 if next_state[i] != "0"
      end
    end
    return false if total_mover > 2
  end
  true
end

def dejavu?(state, history)
  history.each_with_index do |previous, i|
    previous_m = missionaries_state(previous).count("1")
    previous_c = cannibals_state(previous).count("1")
    state_m = missionaries_state(state).count("1")
    state_c = cannibals_state(state).count("1")
    return true if (i%2 == (history.length)%2) && (previous_m == state_m) && (previous_c == state_c)
  end
  false
end

PATTERNS = sieve(cases)
def solve_missionaries_cannibals(state, history)
  history << state
  return true if state == "111111"
    PATTERNS.each do |next_state|
      if possible?(state, next_state, history) && !dejavu?(next_state, history)
        return true if solve_missionaries_cannibals(next_state, history) == true
      end
    end
  history.pop
end

history = []
solve_missionaries_cannibals("000000", history)
p history

良くなった！