実践で学ぶオブジェクト指向③ メソッドの粒度

メソッドを小さく分ける

前回のコードには問題が残っています。Judgeが本来の責任に加え、処理分岐の知識を持ってしまっていることです。

これは良くないので、分岐に関する知識をGame_Masterに戻しましょう。以下のようになります。

  def open(card_id)
    if counter.limit?(opened)
      board.face_down(opened)
      player.face_down
    else
      i = card_id * 3
      if cards[i] == "-"
        board.face_up(i)
        player.face_up(i)
      end
    end
    
    if counter.limit?(opened)
      if Judge.new.judge(cards[opened[0]+2], cards[index(card_id)+2])
        player.face_down
      end
    end
  end

i = card_id * 3をメソッドに切り出したので1行減りましたが、やはりif分岐の中でさらにif分岐しています。インデントが深くて気持ちが悪いです。


そこで、このopenメソッドを細かく切り出してみたいと思います。

openメソッドは大きく2手順に別れています。

• 実行時にopenedになっているカードが2枚あるかどうかによってカードを裏返すか、ユーザーがクリックしたカード(card_id)を表にするようboard（とplayer)に依頼。

• その後、上記の処理によって再度openedが2枚になったかを確認し、2枚になっていたらその2枚を比較し、一致したらopenedを削除するようにplayerに依頼
  
  
  

  ピラミッド建設会社

  
  初めに思いついたのは、openメソッドという名前を保ったまま、中身の処理を細かいメソッドに切り出し、openメソッドではそれらを呼び出す。というものでした。
  
  そして以下のようなメソッドがGameMaster内に追加されました。

def face_up_selected?
  i = card_id * 3
  if cards[i] == "-"
    board.face_up(i)
    player.face_up(i)
  end
end

def already_limit?
  if counter.limit?(opened)
    board.face_down(opened)
    player.face_down
  else
    face_up_selected?
  end
end

def judge_cards(i)
  if Judge.new.judge(cards[opened[0]+2], cards[i+2])
    player.face_down
  end
end

def now_limit?
  if counter.limit?(opened)
    judge_cards(index(card_id))
  end
end

openメソッドは以下のようになりました。

  def open(card_id)
    already_limit?(card_id)
    now_limit?(card_id)
  end

要するに、openメソッドをピラミッドの頂点としてその下にそれぞれのメソッドをぶら下げていくようなイメージです。


しかし、このコードはなんだかイマイチです。
このくらいの量なら問題にはならないかもしれませんが、もし元々のopenが100行くらいあった場合、それを細かく分けてopenピラミッドにぶら下げたコードはとても読みづらいものになると思います。
画面を何度も上下に行き来しなければなりませんから。


リーダブルコードに「分かりやすいコード」というのは「理解するのに最も時間がかからないコード」だと書いてありました。
openメソッドを細かく分散させる必要は間違いなくありますが、上記の方法は最善手ではないようです。

openメソッドという幻想

そこで、"open"という多数の処理を含んだ大きな処理をすっかり消し去ってしまうことにしました。

def already_limit?
  if counter.limit?(opened)
    board.face_down(opened)
    player.face_down
  else
    face_up_selected?(card_id)
  end
  
  now_limit?(card_id) #追加
end

def face_up_selected?(card_id)
  i = card_id * 3
  if cards[i] == "-"
    board.face_up(i)
    player.face_up(i)
  end
end

def now_limit?(card_id)
  if counter.limit?(opened)
    judge_cards(index(card_id))
  end
end

def judge_cards(i)
  if Judge.new.judge(cards[opened[0]+2], cards[i+2])
    player.face_down
  end
end

openメソッドを削除した代わりに追加した記述は、already_limit?メソッドの最後でnow_limit?(card_id)を呼び出すようにした部分と引数のcard_idのみです。


これに伴ってこの一連の処理の開始点であるPlayer側からのメッセージもopen(card_id)からgame_master.already_limit?(card_id)に変わりました。


以上の変更により、各メソッドが一連の流れとして理解しやすくなりました。

もちろん、シーケンス図を完成させるためにコードを書くわけではない

単一責任原則はメソッドにも適用される

今回チュートリアルで定義されたopenという粒度の大きいメソッドをそのまま流用しました。
そのopenメソッドの存在を前提に考えていたため、なかなかこの発想が自分のなかで出てきませんでした。


クラスやメソッドを作る、つまり何かしらの命名をするというのは「処理のまとまり」を作るということです。
それは場合によっては間違った形で思考を制限する危険性があります。
今回それを痛感しました。

最後に、改めて（クラスだけでなく）メソッドも単一責任にすることのメリットを書いておきましょう。(p52～p53要約)

メソッドがそれぞれ単一の責任を果たすことによって、クラスが行っていることがより明確になる。
それは、「いくつかのメソッドを別のクラスに切り出す」という決断の助けになる。
つまり小さいメソッドは設計の改善に対する障壁を下げる。
さらにメソッドの再利用もできるようになる

ところで、処理の中で常に渡されているcard_idという値があります。
これが不必要な箇所でも何度も登場していて鬱陶しいので、次はまずこれをどうにかしようと思います。

あれ……？

処理の流れに沿って、小さく分けたメソッドの最後で次のメソッドを呼び出すようにしました。

この記事を書き上げたあと、これだとメソッドの再利用ができないということに気付きました。

def already_limit?
  if counter.limit?(opened)
    board.face_down(opened)
    player.face_down
  else
    face_up_selected?(card_id)
  end
  
  now_limit?(card_id) #これ
end

今回は神経衰弱のプログラムであり、手順が追加されることはないのでこれでも良いかもしれません。
でも実務でそういうプログラムを書くことはあんまりなさそう……。


処理の流れを知っていて、順番に各メソッドを呼び出す別のメソッドを作ったほうが良さそうですね。
そしてそれはメソッドではなくてクラスとして作ったほうがいいのか……？



他に気づいたことを追記。

• GameMasterじゃなくて単純にGameっていう名前でいいかも

• 「責任」っていうどうにでも解釈できる言葉じゃなくて、完全に役割でクラスを考えたらどうだろう。
  例えば「データ保持役」「処理の流れを知ってる役」「分岐条件を知ってる役」みたいな