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

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

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

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

• BaggageHandler →BaggageHandlerSubject
• BaggageHandlerSubject.BaggageStatus()→BaggageHandlerSubject.ChangeBaggageStatus()
• ArrivalsMonitor→ArrivalsMonitorObserver

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

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

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

完成品

良かった点

• オブザーバーパターンが実際にどう動くか理解することができた。
• 設計力を高めることができた。
• 公式リソースでも、コードを信用してはいけない、実際に動かして改善すべしということを学べた。
• イチロー選手が危惧する「頭で理解しているだけのケース」（いわゆる「完全に理解した」状態）から抜け出せた。

反省点

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