オブジェクト指向らしい判定ロジックの書き方は？

Question

この記事で、if条件を判定するメソッドをクラスのメソッドにするという改善方針が示されています。
https://devtab.jp/entry/internal/27

しかし記事にある「Ｃ．オブジェクト指向設計らしい改善」のようなクラスだと、+n日したり、日付を特定フォーマット文字列に変換したり、年部分のみ取得したりという、もともと日付に実装されている様々な機能を呼び出せなくなるのではないでしょうか？

対策としてすぐ思いつくのは以下手段ですが、もっと良い手があるのでしょうか？

1. this.dateをpublicなメンバとして公開する
2. LocalDateの持っているメソッドについて、利用したいもの全のラッパメソッドを書く
3. 移譲ではなくLocalDateを継承したクラスを定義する (こうすべきなら、記事はなぜ移譲を使った書き方になっている？)

Answer 1

言語が何か書いていないですが、たぶんJavaっぽいのでJavaなのでしょう。LocalDateは、たぶんJavaのjava.time.LocalDateの事なのだと思います。これらを前提として話します。

さて、JavaのLocalDateはfinalクラスです。そのため、継承したサブクラスを作ることができません。また、Javaの委譲は(lombokの@Delegateでも使わない限り)一つ一つメソッドを定義する必要があります。これは極めて面倒です。ということで、2.と3.は現実的ではありません。

最後の1.ですが、Javaにはプロパティという概念が無い(C#とかと比べると理解できます)ため、フィールドをそのまま公開することは悪手であると考えられています。これは将来の拡張や変更において、クラス内の実装を変えられなくなるからです。これは、よくある上書きされてしまうと言った問題とは別の問題です(LocalDateは不変オブジェクトであり、コードではfinalになっているので、上書きや変更される心配はありません)。現実的なのはgetterのようなもの(getDate()というメソッド名である必要は全くありませんが、そう付けてはいけないというわけでもありません)をメソッドとして追加することぐらいです。ということで、1.もあまり良い方法とは言えません。

ということで、1.2.3.ともにあまり良くない手法か、現実的では無いと言えるかと思います。

---

記事の内容についてですが、私的な見解を述べておきます。

記事のコードでの条件文の中の論理式をメソッドにすること自体については悪いことでは無いと思います。ただ、何でもかんでもメソッドにしろというのは極論過ぎます。メソッドにするかどうかは、それ自体がまとまりとして意味があるのかが重要と考えています。

例えば、

if (date.isNotSummer() && Locale.JAPAN.equals(locale))
    ...

という感じだったらどうでしょうか？isJapaneseNotSummer(夏じゃない日本！)みたいなメソッドをまた作るのでしょうか？その場合のレシーバはどうなるのでしょうか？date？locale？となってしまい、ルールを守ることに私には意味が見いだせません。コードは極論ではかけません。バランスこそがいいコードだと思っています。

次に、果たしてLocalDateを委譲するServiceDateで実装するのが良いかどうかです。プログラミング全体を見ていないので確定したことは良いかもしれませんが、私はまずい雰囲気が漂っていると思います。実装すべきなのは、「この日は夏であると決めているクラス」にだと思います。SUMMER_STARTとSUMMER_ENDというのがでていますが、わたしはこの二つが定義されているクラスにstaticで実装するほうがいいと思っています。なぜなら、夏とは何かを決めているのはその場所であり、そこに「この日は夏なのか」と問い合わせればいいからです。責任は誰が取るかというのが設計において最も重要なことです。

記事は、完全に間違ったことを言っているわけでは無いのですが、例もよくありませんし、極論過ぎるところが見られます。JavaなのにJavaの標準のスタイル(Sunが書いたJavaのスタイルガイド)では無いと言うことも、なんか、Java知っているの？という雰囲気を醸し出しています。インデントが揃っていないところとかもありますし。

---

私の経験上、第三者がコメントでツッコミできない技術ブログの8割は内容が怪しいです。このような記事は、眉唾物として、あまり信用しないことをお勧めします。

Comments

[catsforepaw]

言われてみると、元記事では「データとロジックを同じクラスに置く」ことを最大の設計指針としているようですが、責務によるクラス分けという概念が出ていないようですね。
私もその辺をよく注意して見ないといけませんな。

Answer 2

こんにちは。

しかし記事にある「Ｃ．オブジェクト指向設計らしい改善」のようなクラスだと、

（後略）

おっしゃる通り「Ｃ．オブジェクト指向設計らしい改善」は汎用性が劣化していると私も思います。
グローバル関数（フリー関数とも言う）へ分離するだけで、これらのデメリットを回避できます。（javaやC#ならstatic関数）

また、もし1～2回しか使わず、特に重要な判定でもないならベタ書きでも十分と感じます。
高々1～2回しか使わない１行で済む条件判断文を全てグローバル関数にしたりクラスに分離したりしたソースをデバッグする時泣きたくなります。
関数名が中身と一致していることをコンパイラはチェックしてくれません。デバッグ時は全てを疑う必要があります。その時は関数名も信用できないため、一々中身を読む必要があります。べた書きならその場で確認できるのに別定義だとそれを開いて、パラメータの渡し方を間違っていないか等も含めてチェックが必要になります。たった1～2回しか使われていない1行のために数行チェックしないといけないとか涙です。そんなのが10箇所もあったら破り捨てたくなることもあります。

ところで、isNotSummer()は「夏」を判定したい時、!isNotSummer()で２重否定になります。２重否定はなるべく使わない方が可読性／メンテナンス性は上がります。isSummer()の方が分かりやすいと感じます。

Comments

[momon-ga]

補足に書いてありますね。
＝＝＝
実は、このサンプルのロジックは、南半球では使えません。 また、isNotSummer() というメソッド名は "Not" がわかりにくい感じがします。 そもそも夏季・冬季の判断を、 if-then-elseで場合分けするのが、ほんとうに良い設計なのかも、議論の余地があります。 ここらへんの話しは、次回で書こうと思います。

[Chironian]

なるほど。「感じがします」ってことは、この筆者さんは何故分かりにくいのか言語化できていないのですね。ちょっと寂しいかも。

Answer 3

しかし記事にある「Ｃ．オブジェクト指向設計らしい改善」のようなクラスだと、+n日したり、日付を特定フォーマット文字列に変換したり、年部分のみ取得したりという、もともと日付に実装されている様々な機能を呼び出せなくなるのではないでしょうか？

はい。そのとおりです。
ただ、そもそも前提としてあるのが、ServiceDateクラスはLocalDateクラスじゃないということです。
日付の演算なんて本来不要(として設計されている)なのです。

LocalDateの持っているメソッドについて、利用したいもの

という発想自体が、設計として誤っているというか、必要なメソッド(夏季の判断)以外は公開する必要がないです。
極論いってしまえば、内部の日付はLocalDateでなく文字列でもいいです。

元記事にもありますが、一番重要なのは、

データとロジックは同じ場所に置く。

というところです。

<削除>ちなみに、本筋と関係ないので、書いていないのだと想像しますが、
コンストラクタで、dateを直接保持するのでなくcloneとか別のオブジェクトとして保持させるはず。</削除>

クラスは不変でスレッドセーフなら問題ないです。

Answer 4

元記事ではdate変数がどのような使われ方をしているのか、全体像が判らないので想像するしかないのですが、「オブジェクト指向らしい設計」では、夏季でないかどうか（メソッドの論理が逆だとややこしいですね）の判定をするたびにServiceDateオブジェクトを作成する必要があります。元記事ではisNotSummerに引数がなくなると書かれていますが、その代わりにnewする手間が増えてしまいました。

また、ServiceDateクラスのdateフィールドにdate変数のオブジェクトをそのまま渡しているので、date変数とServiceDateオブジェクトとで異なるオブジェクトであるにもかかわらず、その中身は同じインスタンスを参照しているという状態になり、思わぬバグの原因になりかねません。「関連するロジックとデータは同じクラスに置く」つもりなのに、その実体は別の場所にもあってそっちはそっちで管理しないといけない、というのでは無駄にややこしくしただけです。

このようなケースでは、C#なら「拡張メソッド」という手法ですんなり解決できるのですが、Javaには詳しくないのでJavaでも同じようなことができるのかはよく判りません。次善策としては、質問者さんの提示された案3（LocalDateクラスを継承ししてisNotSummerメソッドを持った新たなクラスを作る）ですかね。

ただし、元記事はリファクタリングの話なので、プロジェクトの進行具合によってはあまり大きな変更はせずに、共通で参照できるユーティリティークラスを作ってpublicなstaticメソッドとしてisNotSummerを実装することにするかもしれません。

いずれにせよ、改善のために「オブジェクト指向らしい設計」にしたつもりが、それによって工数やバグが増えたら本末転倒ですので、ご利用は計画的に。

---

訂正
LocalDateは継承できないそうなので（raccy さんの回答で知りました）、その部分は読み飛ばしてください。

Answer 5

「オブジェクト指向らしい」という言い方が具体的に何を指すのか曖昧なところがありますが、
記事の言葉を借りて言うなら「データとロジックは同じ場所に置く」ということを伝えたいのでしょうかね。
ただどうも、このサンプルは何か大きなシステムの一部を誤って抽出したような印象を受けます。

ServiceDate というクラスの名称から察するに、おそらく isNotSummer() とは
夏季休業日であるとか、料金設定が異なるとか、他のビジネス上の要件の一部であると見えます。
仮にもしそうであるなら、正月休みであるとか、ゴールデンウィークであるとか、
関連するいくつかの似たような情報をまとめたクラスであると考えることができます。

元記事で比較対象日付をメンバに含めているのとは発想が逆になりますが、
下のようなコードなら、サービス上の日時と金額に関する情報を一元的にまとめており、
意味を持っていると思います。

class ServiceDate
{
   private LocalDate summerStart;
   private LocalDate summerEnd;
   // ... 以下、その他の設定可能な休日フィールドが続く

   void setSummer(LocalDate summerStart, LocalDate summerEnd)
   {
      this.summerStart = summerStart;
      this.summerEnd   = summerEnd;
   }

   // ... 以下、その他の setterが続く


   boolean canDoServiceOn(LocalDate d)
   {
      if (isSummer(d)) return false;
      if (isYearEnd(d)) return false;
      // その他の判定があればここで行う

      return true;
   }

   // 割増料金。上のメソッドとは矛盾してますが、一例として。  
   int getExtraPriceForDate(LocalDate d)
   {
      if (isSummer(d) ) return this.summerExtra; // 夏季期間割増
      if (isYearEnd(d)) return this.yearEndExtra;
      return 0;
   }

   boolean isSummer(LocalDate d)
   {
      if (d.isBefore(this.summerStart)) return false;
      if (d.isAfter(this.summerEnd) ) return false;
      return true;
   }
   // ... 個別の休日判定ロジックが続く
}

また、Java固有の事情としてマルチスレッドでアクセスされる場合は、
synchronized( serviceDate ) { ... } のようにして扱うことで、
不整合な状態でのアクセスを防ぐ使い方ができるでしょう。
再起動せずに設定ファイルを読み直したり、管理側から設定変更したりするような
Webシステムなどでは考えうる設計です。

単にわかりやすさを求めるだけで、同期も特に必要としないなら、
次のように条件式周辺を2行にするだけでも十分に意図は通じるでしょう。

boolean isNotSummer = date.isBefore(SUMMER_START) || date.isAfter(SUMMER_END);
if (isNotSummer) // ...

ただこれでは同じような判定式が点在し、DRY原則に反するというような批判に加えて、
もうひとつ、ユニットテストができないという弱点があります。
金額の計算や膨大な広告費を割いたキャンペーンの開始日など重大な影響があるなら、
assertTrue((new ServiceDate(2017, 6, 1)).isSummer(), ... ) が
通るのかどうかは、テストしておきたいでしょうね。

これは ServiceDate.isSummer(2017, 6, 1) のように
static メソッドとして実装しても対応可能ではありますが、
この設計はあくまでServiceDateが複数存在しないことが条件となります。
営業所や店舗等を複数束ねる全社統括管理のような大きなシステムでは
各支店ごとに営業日が異なる可能性があり、1つでは都合が悪いかもしれません。