初めに
はじめまして、ココネ株式会社でUnityエンジニアをしているmaphyです。
ココネアドベントカレンダー2023年の20日目になります。
このアドベントカレンダー、実は昨年、新卒の僕が業務の中で「そういえばアドベントカレンダーとかやってないんですかー」と軽い気持ちで当時のCTOに聞いたら「いいね、やろう」とその場のノリで決まったもので、「言い出しっぺの自分が書かないわけにはいかないっしょ」ということで今年もつらつらと書いていこうと思いますw
何について書くか本当に悩んだのですが、新卒2年目ということで業務にも慣れてきた今、自分なりのクソコードの誕生背景についての考察と対策について述べていこうかなと思います。(まだまだ駆け出しのエンジニアが言うことなので、何か間違っている箇所などありましたら遠慮なくご指摘ください！！)
また、あくまでUnityクライアント観点での考察になるので、サーバ側となるとまた少し違う理由や事情があると思います。Unityクライアント観点となるので、この記事の中では基本的にUnity C#での実装を前提としたような表現をしている箇所があります。しかし本質的にはUnity C#に限らない話になっていると思います。
クソコードがどうして生まれてしまうのか
大前提、最初からクソコードを書こうと思って書いているエンジニアはこの世で誰1人としていないと思います。それぞれが頑張ってその時のベストなコードを書いていると思います。
しかし、それでも読みにくい影響範囲が特定しにくいコードというものが実務では生まれてしまいます。
では、どういう時にクソコードが生まれやすいのかということを約2年ほど業務コードを触っていて感じた観点で述べていきたいと思います。
1. お金を生み出す部分(=コアドメイン)
業務=ビジネスでは、やはりお金を生み出す部分については常に新しい機能や体験、それに伴う仕様変更が頻繁に発生します。
例えばスマホゲームであれば、
- 課金に関連する新商品(お得なパッケージ商品など)
- 課金を誘発させる機能(いくら以上の課金で追加で貰える特典)
- ゲーム内通貨を使わせるイベントや新しいガチャなどの実装
- ゲーム内のアイテムを買わせる動機となる既存機能の改修・仕様追加
などが考えられます。
このような箇所については何度も仕様変更・追加が入ることで仕様そのものが複雑だったり、開発メンバーの異動などで把握しきれていない仕様が出てきてしまったりします。その結果、当初は意図されていなかった遷移や処理などが発生してしまうケースが出てきます。
また、ビジネス的な優位性を保つためにもこのような箇所のアップデートはスピーディーに行い、市場に投入する必要があります(特にサービスが軌道に乗る前の初期段階)。その結果、どうしても時間が取れず雑な実装を行なってしまったり、リファクタリングする時間が取れなかったりするケースがあり得ます。
このように「お金を生み出すコアな体験・機能部分=よく改修が入る箇所=開発工数が取れない」という図式が成り立つかなと業務を通じて感じました。
そして、このような部分の実装はよく改修が入るが故に徐々に雑なコードが増えていき、毒のように私たちエンジニアを徐々に苦しめます。
2. いろいろなところから呼べる汎用処理
汎用処理は便利な一方で、特殊なケースが仕様変更などで発生した時にそれに伴った特殊処理を入れざるを得なくなってしまうようなことも考えられます。
こうして追加されていく特殊処理が増えれば増えるほど、コードの複雑性を増大させ、把握が漏れてしまいテストしきれなくなるケースが出てきてしまいます。
3. アーキテクチャ上、誤った書き方をしてしまっている部分
自分が現在携わっているサービスではMV(R)Pアーキテクチャを採用しています。そのため、基本的にはPresenter層がModelとViewを結びつけるように実装し、Viewの変化やModelの変化についてはUniRxを使って伝達するように実装しています。
しかし、この伝達をUniRxではなく、ActionやFuncで行うようにしてしまうとPresenterではなくView側から処理を追っていく必要が出てきます。
そうなると，我々エンジニアは最終的にどのメソッドが呼ばれるのかを追う際に定義元へのジャンプを何度か繰り返す必要が出てきてしまいます。
これはあるメソッドの変更による影響範囲を調査したい時にその影響範囲の特定作業を困難にするケースがあるなと感じています。
また、ActionやFuncを渡すためにコンストラクタやSetup処理などで引数が増えてしまい、時には10以上の引数をとるメソッドが出来上がってしまい、コードが読みにくくなるケースもありました。
4. テストコードがない
クライアントでテストコードを書くとなると経験している人も少なく、
先輩エンジニアたちも「やったことないし、テストコードの保守やテストコード自体の網羅性が本当に担保できるのかなどいろいろ不明なことが多い」となり、やった方よさそうだけど特にクライアントは難しそう。という結論になってほとんどテストコードがなかったりします
しかし、その結果として複雑な仕様を満たしているかのチェックや開発メンバーの移動に伴って失われていく仕様に関する知識などが増えていった時に正しい挙動や不要な処理の判断などができずにコードのリファクタリングが進まないことが考えられます。
問題のあるコードのリファクタリングをしたいとなったとしても、大規模なQAチェックをするコストを考えてもテストコードがない場合はリファクタリングすることが実質できなくなります。(デグレリスクを抱える)
その結果、ずるずるとクソコードが放置されてしまうことがよくあります。
対策について
私の個人的な見解としては以下のようなことが考えられるかなと思っています
- コアドメインについては特にSOLID原則を守るように書くことでコードの柔軟性と堅牢性を担保する
- 適度に仕様に関するコメントを残す
- コードレビューでアーキテクチャ上誤った書き方については忖度せずに伝える
- 過度に共通化しない
- テストコードを書く
1. コアドメインについては特に強くSOLID原則を守るように書くことでコードの柔軟性と堅牢性を担保する
コアドメインこそ、一番頻繁に仕様変更が入る箇所であり複雑になりやすいというのは先に説明した通りです。
そして複雑になるということは、コードが密結合になっていて複数の責務を持ってしまったメソッド・クラスが誕生していたり(単一責任の原則違反)、仕様変更のたびに全体の動作を確認しないといけない処理があったり(オープン・クローズド原則違反)、インターフェースが分離されていなくて意図しないところから値を変更できてしまったり(インターフェース分離の法則違反)します。
なので、SOLID原則を意識したコードを書くようにすることで柔軟性と堅牢性の高いコードを書くことができると思います。
2. 適度に仕様に関するコメントを残す
コメントについては残さないようにした方が良い派と残した方が良い派に分かれると思います。個人的には「各種命名を見て仕様がわかるようにするのが究極的には良い」と考えています。ただ現実的には工数や仕様の複雑さなどによってそれが満たされることは難しいとも思います。
また、特殊な仕様事情によってやむを得ない処理を書くこともまあまああると思います。そのため後から改修するときにどういう仕様になっているかについて確実に辿れるようにコメントを残すのが良いかと思います。
3. コードレビューでアーキテクチャ上誤った書き方については忖度せずに伝える
コードレビューの心理的安全性が保たれているチームであることが前提となりますが、チームで定めたアーキテクチャーや設計的にあまりよくない実装方法になっている場合はたとえレビューイが先輩や上司だろうと意見していくことでチーム全員でコードを保守していくという意識が重要だと思います。(自戒)
明らかによくないケースであれば簡単ですが、そうでないケース(安直にフラグ変数やswitch文での分岐を増やす)では「動いてるし、よくない？」となりがちだと思います。これはその箇所の重要性によっては「動いているからヨシ」でも妥協して良いポイントだと思います。
一方で、分岐の増加は負債につながるので可能であれば別の変数の状態による分岐や呼び出すメソッドを別にするなどで対応した方が良いと思います。
4. 過度に共通化しない
過度な共通化は結果として、あるケースでしか呼ばれないメソッドが存在してしまったり、特定のケースによる分岐ができてしまい、単一責任をむしろ違反してしまい、結果としてもし改修が入った際に影響範囲が広がりすぎます。
5. テストコードを書く
これが一番重要な点かもしれません。コードのリファクタリングについてはその箇所の改修があった際に改修前の設計想定ではどうにもならない箇所があればその際にリファクタリングをすることで、堅牢で変更容易性が高いコードになると思います。
しかし、そのリファクタリングの結果デグレを引くおこすぐらいであれば、リファクタリングを行わず、無理やり既存のコードを修正してなるべく影響範囲を減らすことを考えるべきだと思います。
この心理的な不安を取り除くためにもテストコードとして既存の仕様についての期待する挙動を残しておくのは重要であると考えます。(そもそもテストコードのないリファクタリングはリファクタリングではないよねという話は見て見ぬふりをしてくださいw)
まとめ
業務でコードをいじっていて感じたクソコードになりやすい箇所とその考察でした。
現実的な運用やそこに裂ける工数などの制限があるので必ずしも全ての箇所について理想的な状態である必要性はないと思います．
重要なのはあくまでサービスのアップデートが高速で行えることであると思います。しかし、悲しいことにサービスで重要な箇所ほど変更が入ることが多く、徐々に高速でのアップデートが難しい状態になっていきます。そうならないように重要な箇所ほど柔軟性と堅牢性の高い保守性の高いコードにしておくことが望ましいと思います。
基本的に僕の思考の垂れ流しに過ぎないのですが、もしこの記事が誰かの参考になることがあれば幸いです。
あと、白状すると当日まで全然着手してなくて深夜テンションで急いで書いたので誤字脱字とかロジック崩壊しているところあったらすみません...