同じテーブルの値でも違うクラスを用意すると良い感じ #Java

はじめに

同じ値だからと言って同じクラスを使うだけで良いかというとそうでもなかったな、という話をまとめる。

それぞれ微妙に違う３例を書いてみる。どれも処理内容は適当に変えてるけど、自分が保守しているサービスで苦戦した箇所と同じ構成にした。

本当はクラス設計とかは DDD の考えに基づいて決めるんだけど、今回はシンプルに型を工夫するだけで結構良くなるよ、という話に留める。

記事の前提

アクティブレコードみたいに主キークラスが用意されたりとか言うフレームワークではない
掲載コードは超超簡略化されて要所だけを抜粋している
- 全部実際にはもっとパラメータが沢山あるし、クラスも複雑に分割されている
- 処理ももっと複雑で、メールを飛ばしたり課金請求処理が入ったりする
Service, Tableを除くクラスを便宜上domainsと言うことにする
domainsはServiceやTableに依存してはいけない（domainsからdomains以外に矢印を引いてはいけない）

ケース1 ２つの主キーを扱う

例えば社員に貸与している会社携帯がぶっ壊れて交換するとする。
今使っている製造番号を受け取って、新たに用意した製造番号と置き換える。

クラスとか実装とか

(SerialNumber)とか(SerialNumber, SerialNumber)のところがバグりやすい。

２つの製造番号が変数名でしか区別されていないので、(changed, changed)とかでもコンパイルが通る。。
そんでおそらく第一引数でデータ検索して「そんな製造番号は貸与中ではありません」みたいな実行例外なる感じ。
（今気付いた余談だけど、エディタの補完でぽんぽん書いてると、changedもcurrentもcからだから間違えたりするかも。こういうミスも長い単語だと案外ある）

リファクタリングする

domainsは番号クラス１つしかないので対処はシンプルで、currentとchangedって変数名で頑張っている箇所を全部CurrentとChangedというクラスで識別する方法にリファクタリングする。

良い感じになった。

実際には

最終的には同じテーブルの同じカラムに行くので、ぎりぎりまでデータベースに近いレイヤーまで行ったらSerialNumberをCurrentSerialNumberから作って抽象度を上げても扱っても良いかも。

ただしSerialNumberはdomainsではなくてTableの近くに用意して、CurrentSerialNumber.toSerialNumber()ではなくSerialNumber.from(CurrentSerialNumber)とする必要がある。
toで生成するとdomainsがTableに依存するし、domainsがSerialNumberを使うことを許してしまうことになるため。

ケース2 １つのクラスの状態更新をする

例えば会員管理のシステムがあり、会員の退会を受け付けるとする。
退会するときは状態を退会済みにして空欄の退会日を埋めて状態更新をする。

クラスとか実装とか

Statusの整合を意識するのが大変なのと、0..1のところがかなり実行例外になりやすい。

find(id)から取れたのがどんな会員かわからないので、一応チェックしている。

その後会員の状態を退会にして永続化する時も、設定を間違えたりすると退会ではない何かが永続化されたりする。

リファクタリングする

こういう状態更新系は状態毎に違うクラスにするとかなりの地雷を排除することが出来る。

退会済み会員は利用中会員からしか作れないし、利用中会員は必ず退会日を持たず、退会済み会員は必ず退会日を持つ。

find(id)した結果は利用中会員であると信頼できる。（当然Tableが代わりにチェックに責任を持つ）

Table.unSubscribe()は退会のためにだけあるメソッドなので、Status.UnSubscribedを渡す必要がなくなりシステム都合の設定値がdomainsからなくなった。

実際には

申込中や契約形態の変更中、料金未納や凍結中等のいろいろな状態があるし、管理しないと行けないのは会員だけではなく様々なクラスがある。
全部の状態を同じクラスで設定値だけで使い分けようとすると、設定値の矛盾チェックやOptionalチェックにまみれて実行例外が死ぬほど辛い。

型のある言語を使っていても、コンパイルの恩恵をまず受けられなくなるので、あれ僕何語書いてるんだっけ...??みたいな気分になる。

ここの設計はつきつめるといろいろあって、イベント設計とかも考えると長くなりすぎるので導入だけ。
（というかイベント設計はまだ勉強中...）

ケース3 共通の親クラスがある

例えばユーザの操作を全部操作履歴として残さないといけないとする。
購入を行った場合、購入の申込履歴を保存する。
キャンセルされた場合は申込の履歴をキャンセル済みに更新して、キャンセルの操作履歴を作る。

操作	申込履歴	キャンセル履歴
申し込む	出来る	存在しない
キャンセルする	キャンセル済みに更新	出来る

ややこしいけど、こういう感じ。

クラスとか実装とか

ApplicationとCancellationで違う扱いをするのでクラスを分けたけど、操作履歴として汎用的に扱えるのも便利なので親クラスを作ってしまった例。

やるべきことが複雑なので設定値が多いし、全てを.save()で保存するので値や回数を間違える可能性が多々ある。

リファクタリングする

共通クラスは残しつつある程度申込とキャンセルをコンパイルで正しく扱うには、親クラスを分けてみると良さそう。

退会と同じ様に専用メソッドにすると設定値は大体なくせる。
.save()を.cancel(applied, cancelled)にしたことで必ず２つを正しく更新することが出来る。

実際には

最終的に操作履歴はキャンセルか完了に落ちる。
申込のあと手元に届いたのを確認して利用開始になったりする。
キャンセルの履歴を別で作るのは、これから返金とかを受け付ける必要が新たに発生するからで、キャンセルも必要な処理が終わると完了になる。

イメージ的には最終形はこんな感じ。

操作	申込履歴	キャンセル履歴
申し込む	出来る	存在しない
手元に届く	完了する	存在しない
キャンセルする	キャンセル済みに更新	出来る
返金する	キャンセル済みのまま	完了する

基本的にはこの形だけど提供しているサービスの数だけ操作履歴があって、例えば会員情報更新とか追加備品購入とかいろいろある。

毎度毎度これらを正しく更新するのは極めて難しいので、ある程度の親クラスを用意しつつコンパイルのパワーを受けたい。

まとめ

基本方針

設定値ではなく専用クラスや専用メソッドを用意することで設定値自体をなくす
変数名がシンプルでも大丈夫な様に考える
同じ型の変数が同じスコープに存在しないようにする

やばそうな予兆

メソッドに同じ型の引数が複数ある
とりあえず親クラスを作る
- 間違ってメソッドに渡してもコンパイルが通るから
- それを防ぐにはinstanceofとかでキャストをすることになるけど、それも微妙
- 利用中会員と退会済み会員の親に会員を作るとかもよくあるパターンだけど辞めた方が良い
変数名で用途を伝えようとする
設定値が多い

対処

仕様毎にどんどんクラスを作る
- そんなのしたらクラスが超増えると思うかも知れないけど、増やすべきなのだ
- 属性を和集合的に全部を保持しなければならなくて辛すぎる
- 全部Userで済まそうとするとどんな仕様変更でも常にUserを改修することになり、結局全仕様再テストになるから
- 仕様毎にXxxUserを作る方が影響範囲が小さい
実行例外が出る箇所を決めて、そこに押し込めきる
- 今回はコードは割愛したけど、Tableはデータ不整合とか通信断絶とかあるので例外チェックはちゃんとやる
- Serviceは仕様書に基づきやるべきことをやるので、実行例外のチェックではなく振る舞いに集中する
ユーザの入力を基に正しく設定するべきパラメータと、システム都合で必要なパラメータに分けて属性を考える
変数名がシンプルでも情報が十分で型安全である様に考える

普段の基本スタンスとするべきこと

間違えることが出来る箇所は間違いが起きると考える
- こんなの間違えないだろと思うのは書いた瞬間だけ
- 掲載例は超超簡略化されているだけで、実際はコードの海の中のどこかに埋もれているという点も忘れない
他人からそういうコードが出てくると、案外色々読まないと理解できなくて結構辛い
巡り巡って未来の自分も信用ならないと考える
- 信頼できるのは間違えることが出来ないコード

なんかそんなまとめ。