Microsoft Docsのオブザーバーパターンのサンプルコードをリファクタリングしてみた

今回はC#に於けるオブザーバーパターンの学習をしました。オブザーバーに情報を提供するサブジェクトには`IObservable<T>`を、情報を受け取るオブザーバーには`IObserver<T>`を使います。ここで`T`は受け渡しする情報です。ということでMS公式の説明を読んでみました。

https://docs.microsoft.com/ja-jp/dotnet/standard/events/observer-design-pattern

すると、2022年の観点ではコードにかなり問題があるように思いました。そこで、腕試しにリファクタリングしてみました。

## コードレビュー

全体的に、なぜかJavaふうのブレースの始まり方をしています。C#としてはブレースを使うにはまず改行なので、見栄えが不統一です。

また、条件分岐が全体的に多く、処理の流れが追いづらいですし、メソッドの処理範囲が不適切に思われるものも見受けられます。

### `BaggageInfo`クラス

このクラスは単純なデータクラスですが、内部値とプロパティ値の名前が微妙に違うので見通しが悪くなっています。今ならばGetオンリープロパティで書くところでしょう。

また、コード全般を見ると`Carousel`に負値が入ることは想定されていません。これなら`int`よりも`uint`の方が適切です。ついでに便番号がマイナスなのもおかしい話ですので`FlightNumber`も`uint`にしましょう。

### `BaggageHandler`クラス

サンプルコードとしては`BaggageHandlerSubject`とした方が伝わりやすいはずです。

ステータス変更メソッドが`BaggageStatus`なのですが、動詞ではないのでプロパティと勘違いするし、何をするのか伝わりづらいので、良いメソッド名ではありません。`Carousel`が0よりも多くデータが入っているか、そうでなくて`Carousel`が0の場合とでコードが分割されており、メソッドを分けるべきです。

`BaggageStatus`の引数も`BaggageInfo`を渡した方がオブ指っぽいですよね。引数も多くなると管理難易度が指数関数的に上がりますし。

### `Unsubscriber`クラス

具体的な型を指定しないで`T`とした方が再利用しやすくなります。

### `ArrivalsMonitor`クラス

やはり`ArrivalsMonitorObserver`とした方がサンプルとしては分かりやすいでしょう。

フライト情報を何故か`BaggageInfo`ではなく`string`として持っているのは謎です。オブ指らしくない書き方です。お陰でフライト番号検索で`Substring`しなくてはならず、メンテ性に欠けるレガシーになりやすいコードになっています。本来なら`BaggageInfo`に`ToString`を実装させて、読み出すのが筋ですよね？

`OnError`も実装したいところですが、どうすればいいかわからないので手を付けません。

`OnNext`も`Carousel`の値で処理が二分されており、処理を抜き出したいところです。

`Example`クラスの`Main`は至って普通ですので、特に言及することはございません。

## 方針

こんな感じで進めましょう。

1. クラスやメソッド名をより分かりやすくする。
1. `BaggageInfo`クラスを抽象クラス化し、その中に`BaggageStatus`メソッドや`OnNext`メソッドの処理を格納するメソッドを作る。それらを継承（実装）した情報追加および削除用データのクラスを作る。
1. サブジェクトのオブザーバーリストやオブザーバーのアイテムリストの操作ができるようにする。具体的には`Add`と`Remove`を追加。
1. 3番のリストは一意性を確保できるコレクションで管理するようにする。

1番は具体例を挙げると以下の通りです。

- `BaggageHandler` →`BaggageHandlerSubject`
- `BaggageHandlerSubject.BaggageStatus()`→`BaggageHandlerSubject.ChangeBaggageStatus()`
- `ArrivalsMonitor`→`ArrivalsMonitorObserver`

2番は、ストラテジーパターンです。サブジェクトでは読み出すメソッドを分けることで対応可能ですが、オブザーバーの場合は入口がインターフェースで決まっているのでそうはいきません。よってクラス自身にどうするか決めてもらいます。今回は抽象クラスではなく、面倒だが制約の緩いインターフェイスを選択します。

3番は、処理をデータクラスに移管するために、付けざるを得ませんでした。

4番は、勉強も兼ねて、`HashSet`と`SortedSet`を利用しています。これらの利点は重複データを弾くことと、`SortedSet`は自動で並び替えをしてくれることです。

## 完成品

https://github.com/NagayamaToshiaki/ObserverPatternLearning

## 良かった点

- オブザーバーパターンが実際にどう動くか理解することができた。
- 設計力を高めることができた。
- 公式リソースでも、コードを信用してはいけない、実際に動かして改善すべしということを学べた。
- [イチロー選手が危惧する「頭で理解しているだけのケース」](https://www.sponichi.co.jp/baseball/news/2022/01/11/kiji/20220111s00001000261000c.html)（いわゆる「完全に理解した」状態）から抜け出せた。

## 反省点

- テストを作らなかった
    - 本来のリファクタリングでは「元コードにテストメソッドを追加する」→「テストパターンが失敗しないように注意深くリファクタリングする」という工程を挟む必要がありますが、それを端折っていきなり新しいコードを作り始めてしまいました。そろそろテスト駆動開発を学習したいと思います。
- フライト情報クラスの最適化
    - 現在は便名とカルーセル番号が同じで行先だけが違うフライト情報が林立していますが、これを1つのフライト情報で表せないかと思います。
        - つまり、`BaggageInfo`に行先情報を追加するメソッド（`AddDestination(string destination)`）を加えたいです。さらに本格的にするには空港コードなどを保持する空港のクラスを作りたいものです。
    - この変更をするには、`IBaggageInfo`にも`IObservable`を追加し、`BaggageHandlerSubject`が`IObserver`を実装しないといけません。これは宿題としておきましょうか。
        - `IBaggageInfo`を`IObservable`にすると、追加はもちろん削除でも`OnCompleted`で通知を送って、終了用の`TerminatedBaggageInfo`を廃止できる（あまりスマートなデザインではないですからね…）という利点もあります。ただ、拡張を考えるとインターフェイスを持つというのを変えない方がいいと思います。
- リフレクションの使用
    - 元の並び順に完全に合わせるため、`BaggageInfo.From`を見なければなりませんが、`IBaggageInfo`には枷になるので追加しませんでした。そのため、`Equals`で`is`を使った型判定とキャストを行っています。これは効率的ではありません。解決策としては以下が考えられます。
        - 妥協して`IBaggageInfo`にも`From`を追加する。今回の場合これが最適解だったように思います。
        - 並び順を`IBaggageInfo.FlightNo`で決定するように仕様変更する。現状の仕様では並び順が名前順なので、フライトが分かれてしまうこともあります。実際のプロジェクトで依頼主と掛け合えるならこのほうがいいでしょう。
- 入門向けではない
    - 複雑なデザインパターンを使用しているため、処理のたらいまわしが多くややこしいです。
    - 方針2番は実用コードではともかく、初心者向けレクチャーでしたらおとなしく条件分岐を使うべきだったかもしれません。そこら辺も含めてオーバーエンジニアリングへの警戒をしていく必要があります。