Skip to content

Latest commit

 

History

History
192 lines (159 loc) · 10.1 KB

リファクタリング戦略.md

File metadata and controls

192 lines (159 loc) · 10.1 KB
Raw

リファクタリング戦略

基本方針

書籍『リファクタリング』で紹介されるような、小さなステップで段階的かつ安全な手順で進める。 なおかつ自動テストがないコードベースに対して、書籍『レガシーコード改善ガイド』紹介されるような、まず自動テストを可能とする接合部を安全に作れるような手順で進める。

また上記のような安全な手順を取っていることがわかるよう、通常業務よりも細かい粒度でコミットログを残していく。

目指す形

RxSwiftを用いたMVVMを目指す。 ViewとViewModelの間は

  • View -> ViewModelのイベント通知はメソッド呼び出し
  • ViewModel -> Viewの状態更新はRxSwiftを用いたバインディング

で行う。

進め方案

  1. リバーシのゲームとしての判定・状態管理がBoardViewに代表されるViewの表示状態と結合しているため、切り離していく。
    1. ReversiGameRepositoryを作成し、ゲームのセーブ/ロードを委譲する。
    2. ViewControllerにvar game: ReversiGameを作成し、ゲームの進行に合わせてgameの状態も追従するようにする。
    3. ゲームの判定・状態管理をReversiGameに移す。
  2. ゲーム全体の進行ロジックをViewModelに移す。
    1. まずMVPのような状態を目指す。ViewModel（に最終的にはなるもの）にViewControllerへの参照を持たせ、処理を段階的にViewModelへ移していく。その最中移していない処理はViewControllerの参照を通して呼び出す。この過程で不必要なpublicが増えるのは妥協し、リファクタ完了後に不必要なpublicを削除する。
    2. RxSwiftを導入して部分的にバインディングでのView更新部分を増やしていく。
  3. 設計を変更する。
    1. ViewModelのインターフェースを介して、各振る舞いをテストで保護する。振る舞い単位でのテストとなるため、手順が複雑になったり、アサーションが多くなるのは一旦受け入れる。可能であればあとでシンプルに修正したい。
    2. テストで保護した範囲を設計変更していく。

設計変更案

Reducerを用いた設計は断念

リバーシのゲームとしての静的な振る舞いだけであれば 今の状態 + アクション -> 次の状態 とReducerを導入した設計にできるが、リセットとプレイヤーモード変更が問題。 どちらも優先的な割り込みとして処理する必要があり、なおかつ状況によっては進行中の処理をキャンセルする必要がある。 （ディスクの描画中のリセット、コンピューターが試行中にManualにモードを戻したケース） 関数的にできる部分の割合の方が少ない設計案しか浮かばないため、Reducerは断念。

状態マシンを目指す

  • ユーザー入力待ち
  • コンピューター入力待ち
  • パス了承待ち
  • 画面描画中
  • ゲーム終了

という状態を行き来する状態マシンとして捉えることができそうなので、Stateパターンを用いた設計を目指してみる。

Stateパターンは状態数の追加に対しては強いが、状態遷移を起こす行動数の追加に対しては弱い。また処理が各状態にばらけてしまい、全体を把握しづらくなるリスクもある。 それに対し、

  • 今回のリバーシにはリファクタリング中、振る舞いを維持するため行動数は増えない
  • リファクタリング前の手続き的なコードよりは、各状態に処理がばらけたStateパターンの方が理解がより容易ではないかと予想する

という理由でStateパターンを用いた状態マシンへの設計変更を目指すことにする。

状態遷移図

設計変更後の状態遷移図

クラス図

設計変更後のクラス図

既知バグ

2つのバグを発見済みなので、まずリグレッションテストを自動テストで準備し、その次にバグを再現する自動テストを書き修正していく。

ディスクを裏返す処理の順番が仕様と異なる: 修正済み

README内「ディスクのアニメーション」に記載されている仕様

  • 左上、上、右上、右、右下、下、左下、左の最大 8 列のディスクが裏返る可能性があるが、ここに列挙した順に、各列内のディスクを裏返す

とあるが、左下と左の順番が逆になっている。

再現手順

通常のゲーム進行中で検証可能な盤面に至ることが難しいので省略。 左下と左の順番が、左が優先されていることだけであれば容易に検証可能。

テストケース

初期値

xxxxx---
xooox---
xo#ox---
xooox---
xxxxx---
--------
--------
--------

の#の位置に黒のディスクを置いた時

xxxxx---
x234x---
x915x---
x876x---
xxxxx---
--------
--------
--------

の順でディスクを置く・裏返すアニメーションが実行されることを期待する自動テストを作成する。

次にプレイヤーの有効な手が存在しない状態でゲームを開始すると進行不能となる: 修正済み

ゲーム中、次にプレイヤーの有効な手が存在しない場合アラートが表示される。 しかしゲーム開始時読み込んだファイルが、次にプレイヤーの有効な手が存在しない状態だと進行不能になる。

  • プレイヤーモードが"Manual": どこにもディスクを置くことができず、ターンが相手プレイヤーにも渡らない。"Reset"ボタンを押すことができるのみ。
  • プレイヤーモードが"Computer": クラッシュする。

再現手順

有効な手が存在せずパスするアラートが表示されている最中にアプリを再起動する。

テストケース

初期値

ox------
--------
--------
--------
--------
--------
--------
--------

で黒のターンで開始すると、相手プレイヤーにターンが移ることを期待する自動テストを作成する。

リセット確認アラート表示中にパスするアラートが表示されない: 修正済み

リセット確認アラート表示中、裏でコンピューターが操作を続け置く場所がなくなると、パスするアラートの表示を試みるが Attempt to present <UIAlertController: *> on <Reversi.ViewController: *> (from <Reversi.ViewController: *>) which is already presenting <UIAlertController: *>. とログが出て、パスするアラートは表示されない。 リセット確認アラートをCancelで閉じてもパスするアラートは表示されず、ゲーム継続不能となる。

再現手順

通常のゲーム進行中で検証可能な盤面に至ることが難しいので省略。

テストケース

おそらくXCUITestも可能だけれど、手動テストで十分リグレッションテスト可能。 ゲーム読み込み部分を一時的に変更し、以下のゲーム初期状態をハードコーディングする。

xo----ox
--------
--------
--------
--------
--------
--------
--------

両プレイヤーのプレイモードはコンピューターで黒のターンで開始する。 1つ目の黒が置かれる前にResetボタンを押しリセット確認アラートを表示しておく。

画面描画中にモードを切り替えアプリを再起動すると不正なゲーム状態に陥る: 修正済み

ゲーム状態の保存は

  • 画面描画完了時
  • モード切り替え時
  • リセット時

に

  • 盤面
  • ターン
  • 各プレイヤーのモード

を全て同時に保存している。また、ターンの切り替えおよびゲーム終了判定は画面描画完了時保存直前に行う仕様となっている。 上記仕様であるため、画面描画開始後〜描画完了前にモードを切り替えると、画面描画途中の盤面かつターン切り替え前の状態が保存される。そのまま画面描画完了前にアプリを再起動すると、

  • ディスクが裏返り切っていない盤面
  • 連続して同じプレイヤーが操作できる
  • ゲームが終了していてもプレイヤーの操作待ち

という不正なゲーム状態を作ることができてしまう。不正なゲーム状態で開始すると行動不能に陥ったり、クラッシュする。

再現手順

ゲーム後半にディスクを1つおくと裏返る枚数が多い状況で、

  1. ディスクを起き
  2. モードを変え
  3. アプリをキル

を急いで実行すれば比較的容易に再現可能。

テストケース

ViewModelでRxTestを用いてタイミングをコントロールすれば、普通の盤面で再現可能。

修正方針

いくつかの修正案が考えられるが、 「画面描画中はモードを切り替えても保存しない」 ようアプリの仕様を変更する。

仕様を維持する修正するには、アプリ内で状態管理をするだけでは難しいため、データ形式を

  • 一手一手行動を記録し、初期状態と行動履歴から最新状態を計算する
  • 保存時盤面とモードを別々に保存できるようにする

と変更する方法が考えられる。 また仕様を変更する修正案としては

  • 画面描画中はモードを切り替えても保存しない
  • 画面描画開始前に保存する

が考えられる。 今回のリファクタリング・チャレンジでの設計変更では、状態マシンに設計変更することで、元々ViewController内で行われていたアプリの状態管理をわかりやすく変更することを主目的として考えている。そのため最も修正による影響範囲が狭いと考えられる 「画面描画中はモードを切り替えても保存しない」 で修正を行う。