「悪いコードから知る変更容易性の真価」について

ttps://zenn.dev/mbao/articles/good_code_bad_code について、
感じたことを書き出していくスクラップ

個人的な意見で、本質的ではないことばかりです

wip

全体的に

X について、A を改良して A' にしました、という流れになっている。
X について改良されたのは理解できるが、
それ以外にも改良すべきではないか？という部分があると、読んでいて気になってしまう。
以下に書いたことは、そういう部分を言っているというか、ややもすると重箱の隅を突いているだけかもしれない。

こういう話で例示されるコードブロックは、かなり難しいというか、注意が必要であるように思う。
結局のところ良し悪しは前提条件にも拠るので、まったく違和感を抱かれないコードブロックを例示するのは容易でない感がある。

PHP の機能を最大限に活かした記述の方が嬉しい

たとえば constructor property promotion とか readonly とか enum とか使って書き換えられそうな箇所がある。
引数や返り値の型くらいは書いておいて欲しい。いまや書かれていないとレガシーにすら感じてしまう（ただ Eloquent まわりで型をしっかり書くのはかなりシンドい印象があるので、そのへんやりはじめると大変かもしれない）。

このへんをしっかりと書くこと（利用すること）は、良いコード悪いコード以前の問題であるかもしれない。

Laravel の文脈が強すぎる気がする

といってもぺちぱーはたいてい Laravel 読めそうなので、ぺちぱー以外の読者を想定していないとも考えられる。
「Laravel だったら」という前提で語るのならば、はてブのコメントにもあったけれどより Laravel らしい書き方ができる箇所がもっとあるかもしれない。

if の条件式は明示的で／適切であって欲しい

これは自分が過激派な自認がある。

        $user = User::find($id);
        if (!$user) {

みたいな例は $user が null であったとき、null は boolean にキャストすると false だから成立するわけであるが、個人的にはこのような暗黙的なキャストが含まれる条件式は好ましくない。

        if ($uesr !== null) {

であって欲しい。が、これはやりすぎだという意見も理解できる。

...

        $product = Product::find($productId);
        return $product ? true : false;

上のように暗黙的なキャスト + 三項演算子を書くのならば、
いっそ明示的に return (bool) $product; として良い気がする。
自分としては以下のように、正しい厳密な比較結果を返したい。

        $product = Product::find($productId);
        return $product !== null;

return Product::where(...)->exists(); でも良いかもしれない。

...

        if (!isset($this->statusHandlers[$status])) {

についても、やりたいことは key を持っているか否かなので、

        if (!array_key_exists($status, $this->statusHandlers)) {

のほうが好ましい。

と、ここまで書いてぺちぱーなら皆気付いただろうが、前者と後者は同値ではない。
isset() は null が格納された変数を false と評価し、array_key_exists() は null の value をもつ key がヒットしたら true と評価する。こういった厳密性を考えても自分は後者の方が好ましい。

ちょっと逸れるが empty() についても自分はあまり好ましくない。が、これもひとによっては逆に便利だと多用されることもあるので、チームの合意に沿うのが望ましいだろう。

FatController ?

規模の小さいリポジトリならば別に全部コントローラに書くのもありだし、トランザクションを張らないのも別に構わないかもしれない。
こういうコードブロックに EnterpriseEdition を求めるのは本質的でないので、これは指摘する方が適当でないかもしれない。

「未初期化状態（生焼けオブジェクト）」

通常 User は UserId みたいな識別子とライフサイクルをもつエンティティで表現されることが多い気がする。あまりユーザを完全コンストラクタ（？）としたいことはないのでは。それならばシンプルな構造体とする「問題のあるコード」のほうが使い道があるかもしれない。

個人的には、こういう類の話はシンプルに、状態は少ない方が良いという方針にまとめて良いと思う。

関係ないけど Complete Constructor とか Half Baked Object とか出典はどこなんだろう。
ちょっと調べたら Complete Constructor は Beck の「実装パターン」にあった。
（「良いコード/悪いコードで学ぶ設計入門」の参考文献に「実装パターン」はなかったので、別にあるのかもしれない）

「ベタ書きせず、意味のあるまとまりでメソッド化」

ただ Laravel の機能を使っただけな気がする。
仮に Laravel の機能などと考えずに論ずるならば、この程度の文量のメソッドをあえて分割するのは間接層 Indirection が増えるデメリットの方が大きいかもしれない。
こういうことを言い出すとキリがないから、やはりこの類のコードブロックの例示は難しいというか、あるいはこういう指摘をすること自体がナンセンスな気もする。

「成熟したクラスへ成長させる設計術」

ユーザがプロパティに today を持つことは流石に違和感がある。
また Today や Birthdate に専用の型を与えるのもやりすぎな気がする。ただ Carbon にしてあげるだけでいいのでは。

仮に Birthdate をクラスにするのなら、年齢を計算するロジックは User より Birthdate にあったほうが望ましい気がする。また today という名前は外部から与えられることを許容しているには限定的すぎるので、criteria みたいな名前でいいかなと。

「初期化ロジックの分散」

これについてはよくわからなかった。あるクラスの生成について、同じ方法を記述している箇所が複数あるのならば、ファクトリメソッドにしてまとめようということかな？
Standard や Premium の文脈によっては、これが Point 側ではなく、生成する側の定数として定義されていた方が適当な場合もあるかもしれない。

コードブロックの例が難しいという話にまた繋がってしまうのだけれど、Point や User のような貧血症を起こしたドメインモデルが例示されていると、そもそもこのクラスの存在意義が判然としなくなってしまい、そのクラスをどう良く扱うかという話もわかりづらくなってしまう気がする。