[Unity]リファクタ作業は大変だねというお話(後編)
前編
13. 神クラス
いわゆるありとあらゆる機能が集約してしまったとても大きなクラス。
もはや何を制御しているクラスなのかはわからず、数千行におよぶコードは可読性が大きく損なわれている。
何か変数を一つ書き換えようものなら、波及して多くの変更を余儀なくされることもしばしば。
複数の機能が備わっているということは、何か実装を変えようと思ったときにはこのクラスに辿りついてしまうということ。
そのために競合が起きまくる。
14. クラス分け
「オブジェクト指向やMV(R)Pパターンを徹底しろ!」と言うつもりは毛頭ありませんが、最低限クラスごとに役割が明確になっていると読む側としては嬉しいなという話です。
UIとキャラクターの制御を同じクラスで行っている、
セーブデータ管理のクラスからエフェクトの再生を行っている、
といった設計は流石に良いものとは言えないんじゃないかな、と..。
15. クラスの相互参照
俗に言うスパゲッティコードの第一歩だと思ってます。
ClassAがClassBを参照し、ClassBもClassAを参照しているというもの。
二つのクラスで済むならまだマシで、参照し合うクラスが三つ以上になると処理を追う難易度が爆上がりします。
処理を追っていたらクラス4つほど経由した後に最初のクラスに戻ってきた時はさすがに驚いたものです。(イベントやコールバックは使っていない。)
16. public
変数のアクセス修飾子は極力狭くする方が良いです。
インスペクターに表示したいのであれば[SerializedField]を、他のクラスからアクセスしたいのであれば関数化するかプロパティにしてしまうかの方法をとる方が良い。
上記のような目的であろう箇所がほぼ全てpublicになっていたせいで、設計がかなり大変なことになっていました。
ありとあらゆる箇所からアクセスし放題の書き換え放題。
意図しないクラスから意図しない値に書き換えられる可能性があるので非常に危ない。
それと、誤って呼び出せてしまうという点でも危険。
さらに、シンプルにコードを追うのが大変になる。
17. partial
partialを使えば一つのクラスを複数ファイルに分けて実装することができます。
クラス内の処理を機能ごとに分割したいときなどに便利です。
ですが、うまく使わないとただソースコードを複数ファイルにばらけさせて分かりにくくするだけになってしまいます。
「この変数がどうあがいても意図していない値になる..。」なんて思っていたら、実はpartialクラスだから知らないファイルで書き換えられていたなんてこともありました。
さらに、ソースコードを役割ごとに分けたくてpartialにしているはずが、それすら守られていないことも。
ファイル1に内包されるべきコードが、ファイル2のほうに書かれているという状況です。
ファイル2のフローを追っていたら、フローの途中だけ何故かファイル1の方に実装されているということがあり、「実装者も把握できていないのでは...」なんて思ってしまいました。
18. static
個人的な感想ですが、staticもバグの温床だと思ってます。
staticである必要がないstaticをいくつも書き換えました。
大半はただ単に「他のクラスから参照するため」だけにstaticになっていたので...。
19. enum
int => enum に変換している系がリファクタ対象になりました。
enumの要素を変えるだけでいきなり機能しなくなる可能性があるためです。
また、inspectorにenumを表示している系もめちゃくちゃ便利ではあるのですが、一歩間違えるとバグの原因になります。
というのも、enumの最後尾以外に要素を追加した場合、inspectorで設定していた値が軒並みずれる恐れがあるためです。
20. Start(),Awake()
MonoBehaviourで勝手に呼び出される関数系統。
この中にいろいろと処理を書いてしまうと、ソースコード側で呼び出しのタイミングを制御、認識できなくなってしまいます。
また、これらの関数内でGetComponentや他クラスへのアクセスを行うのもなかなかに危険です。
21. 継承
overrideで結局ほぼほぼ実装しなおしていたり、継承先では不要な変数関数もまとめて継承していたり、そもそも継承元の実装を全く使っていなかったり。
継承することの旨味を生かしていないどころか、むしろ変に継承していることで手間が増えたり可読性が落ちているケースが非常に多かったです。
どういう実装かというと、剣を制御するクラスが銃を制御するクラスを継承して実装されている感じです。
下手すると剣でリロードや遠距離攻撃ができてしまう可能性があります。
確かに「武器」という点では同じですが、仕様は全く違うのですから継承関係にするべきではありません。
こういったものは、WeaponBaseのような基底クラスを作ることで対応しました。
22. 参照型のたらいまわし
classやTransformを複数のクラスに跨って変更を加えるような実装。
classAで座標を少し上にずらしてからclassBに渡し、classBでは移動処理を行って、classCでも座標の微調整を行う...みたいな実装。
ランタイムで座標を確認したくても、実際に今どの処理を経て現在の座標になっているのか、把握がとても大変です。
わかりやすければ文句はないのですが..。
また、単純に処理が散らばっているということでもあるので、後に変更を加える際に予期せぬエンバグを引き起こしかねません。
23. コルーチン
private void Start()
{
StartCoroutine(CoroutineA());
StartCoroutine(CoroutineB());
}
//
private IEnumerator CoroutineA()
{
yield return new WaitForSeconds(2);
flag_A = true;
while ( !flag_B ) { yield return null; }
Debug.Log("Complete");
}
//
private IEnumerator CoroutineB()
{
while (!flag_A) { yield return null; }
yield return new WaitForSeconds(2);
flag_B = true;
}
コルーチンで待ちの処理を含んだフローを実装するのは構わないのです。
(今ならUniTaskを使っているとは思いますが。)
ですが、それを二つ同時に走らせている上に互いに干渉しあっているとなると、途端にコードを読むのが億劫になります。
例えるなら、二か所から同時に球を転がし始めるタイプのピタゴラスイッチのギミックを全部理解しろと言われている感じでしょうか。
解説=コメントが欲しくなります。
実際には、コルーチンの数が三個以上存在していて、それぞれが別のクラスにばらけていて、そもそものコードがくそ長いのですから、本当に大変でした。
おわりに
改めて、記事内で語った意見はあくまで私の知識と感性に基づいた主観的なものです。
実装と設計の良し悪しはプロジェクトや環境で変わるものだと思いますので、あまり
鵜呑みにしないでいただければと思います。
半ばストレス発散で書きなぐったものなので、終始文章がめちゃくちゃだと思いますがお読みいただきありがとうございました。
Discussion