Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve StockHolder events #74

Merged
merged 11 commits into from
Oct 12, 2021
Merged

Conversation

Siroshun09
Copy link
Member

@Siroshun09 Siroshun09 commented Oct 12, 2021

改善の発端

StockHolder が保存されたときに呼び出されるイベントが StockSaveEvent で、微妙に名前と用途が一致していないことから再検討を始めた。

変更点

  • StockHolderEvent の作成 - StockHolder に関するイベントの最上位クラス
  • StockHolderLoadEventStockHolderSaveEvent の作成
  • StockSaveEvent の廃止予定化
    • 互換性のため StockHolderSaveEvent 構築時に呼び出される
    • イベント名からすれば、在庫アイテムが1種保存されるごとに呼び出すイベントになるだろうが、現行の構造上ストレージ実装部分で呼び出すことになり、本来のストレージの責任範囲から外れることから実装が望ましくないため
  • StockEventStockHolderEvent を拡張するようにする
    • StockEventStockHolder の中の在庫 (Stock) が変更されたときに呼び出されるため

NOTE

StockEventStockHolderEvent を拡張するならば、StockEvent 群は n.o.b.api.event.stockholder に移動すべきである。
しかし、これを行うには、n.o.b.api.event.stock 下のクラス群を v4.1.0 で廃止予定化して v4.2.0 で削除するか、移動してバージョンを v5.0.0 にする必要がある (誰も使ってないはずとはいえ、あくまでも API なので破壊的変更は配慮がいる)。

さすがにそこまでやるのは面倒なので、いつか v5.0.0 に上げるときにパッケージを移動する (TODO としてコメント済み)。

@Siroshun09 Siroshun09 added enhancement New feature or request refactoring labels Oct 12, 2021
@Siroshun09 Siroshun09 added this to the 4.1.0 milestone Oct 12, 2021
@Siroshun09 Siroshun09 self-assigned this Oct 12, 2021
Copy link
Member

@LazyGon LazyGon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

みた
問題なさそうではある

イベント処理をバンバン非同期でやって大丈夫なのかっていう不思議さはあるけど、そもそもイベント処理が独自実装だからまぁって感じ

StockSaveEventはこれStockChangeEventって雰囲気の処理だったのか
StockHolderSaveEventを実装するのはいいけど、そもそものStock変更時のイベントはなくなる感じなのか

全部チェックアウト・プルしてコードを確認したわけじゃないから的はずれなことを言っているかもしれない

いじょう

@Siroshun09
Copy link
Member Author

イベントライブラリは厳密にはスレッドセーフではない (後述) ですが、イベントを非同期で発火する分には問題ないです

StockSaveEventはこれStockChangeEventって雰囲気の処理だったのか

正確に意図をつかめてるかわからないけど、実装当初は StockEvent 自体が StockHolder に関するイベントという立ち位置だったので、StockSaveEvent はそこに入れられた (ついでに、StockSaveEvent は後付けだったのでそんなに深く考えてなかった)

StockHolderSaveEventを実装するのはいいけど、そもそものStock変更時のイベントはなくなる感じなのか

Stock 変更時のイベントは StockEvent を拡張するクラス (StockSetEvent, StockIncreaseEvent, StockDecreaseEvent) のまま変わらず、StockEvent 自体が StockHolderEvent を拡張するようになるという感じですね。
(本当は StockEventStockModifyEvent 的な名前に改名したみはある)

んで、イベントライブラリが厳密にはスレッドセーフではない件

簡単に言うと、イベント処理中にリスナーを追加 (subscribe) / 削除 (unsubscribe) すると ConcurrentModificationException (曖昧) が出ることがある (と予想される)。
まぁこれは ArrayListCopyOnWriteArrayList にするなり、更新処理でリストを新しく生成するなりなどの修正をライブラリに施せばいいので、そのうちやると思う

@LazyGon
Copy link
Member

LazyGon commented Oct 12, 2021

ok 説明が受理されました

@Siroshun09 Siroshun09 merged commit b50374e into v4/develop Oct 12, 2021
@Siroshun09 Siroshun09 deleted the improve/stockholder-events branch October 12, 2021 14:50
@Siroshun09 Siroshun09 mentioned this pull request Oct 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request refactoring
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants