[Unity]リファクタ作業は大変だねというお話(前編)
はじめに
そこそこに規模の大きなプロジェクトでそこそこに大掛かりなリファクタ作業を行う機会がありました。
理由としては、可読性や設計があまりよろしくなくて後々の改修作業や拡張作業に耐えられる作りではないと判断されたためです。
正直、かなり大変でしたし、なんでこんな実装にしたんだ？と思うことも多々ありました。
ただ、この経験を通していろいろと学ぶことが多かったため、己への戒めも兼ねてつらつらと書き連ねてみます。
主にソースコードの話になると思います。
この記事の目的
まず前提として、リファクタ対象のソースコードなどを批判するためではありません。
当時の仕様ではそれが最善だったかもしれませんし、設計や実装の良し悪しなんてプロジェクト次第でいくらでも変わると思います。
記事内で語る意見は、あくまで私の知識と感性に基づいた主観的なものです。
当然、私が実装の意図をくみ取れていなかったり、誤った知識で判断している可能性も大いにあります。
また、「こういう実装するときはこういう設計にすべきだ！」と言いたいわけでもありません。
改修案などについても、今回は特に詳しく記載しないと思います。
あくまで、私自身の備忘録、そして「自分はこういったところに気を付けて実装をしよう」という自戒のためです。
と長々しく保険をはりましたが、「へ～、そんな作業したんだ。大変だったね笑」程度の軽い気持ちで読んでいただければと思います。
ぶっちゃけ半分くらいストレス発散なので文章がめちゃくちゃです。
1. コメントがない
苦労した原因の半分はこれだったんじゃないかと思います。
クラスやメソッド、変数の説明もどんな意図で実装したのかとか、そういう説明がとにかくありませんでした。
どのくらいないかというと、ちゃんとコメントがあるコードを見ると嬉しくなるくらいです。
コードを読まなければ何もわからないので、とにかく時間が取られました。
「読みやすいコードを書けばコメントなんて不要」という意見もあるかと思いますが、それは読みやすいコードが書けたらの話です。
「変数名や関数名、クラス名を見れば大体はわかるだろ」という意見もあるかもしれませんが、それはわかりやすい命名がされていたときの話です。
ここら辺は後述すると思います。
一応補足しておきますが、何も懇切丁寧な文章で1行1行コメント書けとも思いませんし、コメントがあればソースコードを読まなくてもOKとも思っていません。
ただ、こう、「もう少し手心を...」という気持ちになります。
2. 仕様書が更新されてない
仕様書に記載されている仕様と実際に動いている仕様が違うケースがちょこちょこありました。
おそらく仕様書を更新する暇がないくらい忙しかったのだと思います。
その結果、後から見た私にとってはそれが意図した挙動なのか不具合なのかわかりませんでした。
なにより問題だったのが、ゲーム中の計算式や分岐条件が文書として保存されていなかったことです。
当時のプロジェクトに参加していた人が残っていなかったため、上記のような仕様を知るためにはソースコードから読み解くしかありませんでした。
計算や画面フローをソースコードから逆算するわけですね。
これがかなり骨の折れる作業で、かなり時間を取られることになってしまいました。
3. フォルダ構成
新しい仕様のアイコンを追加するにあたって、NewIcon.prefabを作成しました。
種別としては、UIのPrefabです。
それでは、このファイルは下記の二つのフォルダが存在するとき、どちらのフォルダに追加するべきでしょうか。
・Assets > UI > Prefabs
・Assets > Prefabs > UI
答えは、「どちらでも良い」です。
どちらのフォルダにも何かしらUIのPrefabが入っていました。
特に規則性や決まりごとはない様子。
続いて、次のフォルダ構成を見てください。
Root/
├ Hitokage/
│ ├ Texture/
│ └ Prefab/
├ Husigidane/
│ ├ Hitokage/
│ │ └ Texture/
│ └ Texture/
│
└ Prefab/
Root > Hitokage 以下のフォルダは全て空です。
ヒトカゲのテクスチャは Root > Husigidane > Hitokage > Texture に入ってます。
フシギダネのテクスチャは Root > Husigidane > Texture に入ってます。
ヒトカゲ、フシギダネのPrefabは両方とも Root > Prefab に直接入ってます。
何が書いてあるかわからないと思いますが、私もよくわかりません。
他にも、当然のようにScriptフォルダの下にPrefabがあったりと、さすがにやばいという話になりフォルダも整理することになりました。
4. 命名 - スペルミス編
スペルミスなんて私自身も頻繁にしてしまいますし、誰でもやってしまいうる事だと思います。
とはいえ、可読性を阻害する要因であることに変わりはありません。
Cat ⇔ Cut, Date ⇔ Data 等は意味が変わってしまいますしね。
一番困るのは、Controller ⇔ Contrller みたいなよく使う単語のスペルミスでした。
なんで困るかというと、検索に引っかからないからです。
私はProjectタブでファイル名にあたりをつけて検索することが多いのですが、上記のようなスペルミスがあるとこういった検索でヒットせず、フォルダをポチポチ開いて探すことになります。
(前述のようにフォルダ構成がめちゃくちゃなこともあったため、時間がかかることもある。)
5. ファイル名依存の実装
「このキャラクターはピカチュウか？」を判断するif文条件が「オブジェクト名に"Pikachu"と含まれていたら」でした。
他にもオブジェクトの検索を名前準拠でやっていたり。
私たちは前述のスペルミスしているファイルをいくつか修正していましたので、上記のような実装がいくつか動かなくなりました。
ファイル名に依存した実装が必要になる場面ももちろんあると思います。
しかし、その場合は命名規則をしっかり決めたうえで、スペルミスには注意していきたいものです。
6. 命名 - 特定できない編
ファイル名やコード内の変数、関数名はできる限り明瞭にすべきだと思ってます。
（意図的にわかりにくくする人はいないと思いますが。）
下記は実際にあったコードです。
ある画面では、3種類のダイアログが表示可能であり、私はその内の1つに変更を加える必要がありました。
public class DialogConroller
{
public GameObject dialog_1;
public GameObject dialog_2;
public GameObject dialog_3;
public void OpenDialog_1(){}
public void OpenDialog_2(){}
public void OpenDialog_3(){}
}
コードを読めば、読めばわかるんですけどね？
それぞれ変数名でどんなダイアログかを説明してくれれば、本来は一瞬で判断できたはずです。
それに、一回判断できてもしばらく後に読み返した時にはどれがどれだか忘れているので、またコードを読む手間が..。
他にも、下記のようなコード。
これをスクリプトの500行目あたりで見つけたとします。
_dd.Open();
この変数は何なんだと思って宣言部分まで遡ってみると、型はDetailDialogという自作のクラスでした。
オリジナルのクラスを略さないで...。
RigidBodyをrbと略したりするのは何となく共通認識(?)だと思ってますが、自前のクラスを略されると実装者以外には何のことやらさっぱりです。
今回も_detailDialogとでも命名してくれていれば、私は「あ、ダイアログ開いてるんだな。じゃあ今回は関係ないな。」といった具合に、わざわざ宣言部分まで遡らずとも判断できたはずです。
せめて略すならコメントを(。
ファイル名に関しても同様です。
Button.png という画像ファイルがあっても、「どのボタンの？」としか思えません。
画像を見たところで、作業担当者でなければ判断できない場合もあるでしょう。
可能な限りわかりやすい命名を行いたいものです。
7. マジックナンバー
コード内に直接値を打ち込んでいる系。
まず第一にPG以外が触れないので、値を調整するたびにPG作業が発生するのがかなり面倒。
調整担当の人からしてもかなり煩わしいと思います。
そして第二の問題として、修正が必要な箇所が把握が難しいことです。
アイテム所持上限値の999という値が手打ちになっていたのですが、これがいろいろなソースコード上に点在していました。
もちろん全部の箇所を修正しないと、どこかで確実にバグります。
この「999」という数字を探すのがあまりにも困難でした。
ソースコード全体に「999」という数字で検索をかけても「9999」や「999999」という数字も引っかかりますし...。
これが一桁の数字だったらと考えると鳥肌が立ちます。
8. 使ってないコード、ファイル
実装したが仕様がオミットになってしまったコード、今後使うかもしれないので一応用意しておいたコード、プロジェクトの状況次第ではありますが、明らかに不要になったものは消しておいてほしかったり。
他にも、Trueで定義されたまま一生書き換えられないboolean、計算結果が格納されているがその後一度も参照されないintなど、実装内の変数を遡ってみたら実質的には何の役割も持っていないというようなものが多く見られました。
可読性を落とす上に、コードが無駄に長くなってしまうので上記のようなものは消したいものです。
ただ、念のため残しておきたい気持ちはとても良くわかります。
9. テスト用、デバッグ用、調整用のコード
テストやデバッグで使用したであろう関数や変数が我が物顔で、それも大量に本番環境のソースコードに紛れていました。
「よくわからない変数・関数があるからどこで使われているか調べてみたらテスト用の変数だった。」なんてことが多々発生しました。
(せめて名称に _Test とか付けてほしい)
アクセスのレベルもpublicなので、誤って呼び出してしまう可能性がぬぐい切れない。
そして単純に無駄にコードが長くなるので読みにくい。
一番やばいと思ったのは、関数内に本番でも使用するコードとテストでのみ仕様するコードが混在していて、引数の bool isTest というパラメータで分岐させていたこと。
本番ビルドでも平気で if ( isTest ) というコードを通過する。
#if UNITY_EDITORといったスクリプトシンボルではじかれていれば良かったのですが...。
テスト用、デバッグ用、調整用のコードが本番ビルドにガンガン入っている。
おまけにリファクタ中にテスト用やデバッグ用のコードでもりもりコンパイルエラーが出る。
なので分離するにもかなりの工事が必要に。
やばい。
10. 共通して使用するUIがPrefab化されていない
ダイアログやボタン、アイコンなど使用する場面が違っていても、デザインや挙動が同じであればある程度Prefabにして共通化するべきだと思ってます。
テキストだけの通知用ダイアログや戻るボタンといったものは基本全画面で出うるものですが、こういったものが各画面でオリジナルに作られていました。
(おそらくコピペで複製しまくったのだと予想。)
何が起きるかと言えば、「戻るボタンのサイズを少し大きくしたい」という変更が来た時に、全ての画面に対して総当たりしなければなりません。
prefabになっていれば数分で終わるものが、画面の数だけ倍々で時間がかかります。
そのうえで、間違いなく対応漏れも発生します。
そもそも戻るボタンが存在する画面を探すのがとても手間です。
11. 共通して使用するデータが一か所にまとめられていない
アイテムアイコンに使用するSpriteデータは全部で80種類くらいあるのですが、アイテムアイコンを使用する各種画面で個別に設定されてました。
全部で3か所、それぞれが長さ80のSpriteのリストを持っている状態です。
インスペクターに設定する作業も鬼のように大変ですが、何よりメンテナンスがあまりにも重たすぎます。
こういった実装も間違いなく対応漏れを起こします。
メンテナンスを楽にするために、ScriptableObjectなどで一か所にまとめるのが良いのかなと。
12. 順番依存のリスト
List<string> names = new List<string>() { "Pikachu","Raichu" };
上記のリストから"Pikachu"を取り出すときに、names[0]と記述するような実装はあまりにも危険。
なぜなら、Listの中身が変わった瞬間にインデックスを全て書き換える必要が出てくるため。
List<string> names = new List<string>() { "Pichu", "Pikachu","Raichu" };
Listが上記のように変わった瞬間、names[0]と記述されていた箇所は"Pichu"を参照するようになってしまう。
マジックナンバーの項でも触れたが、修正漏れがあった瞬間にバグるのでとても危険。
特にインスペクタでListを作成しているタイプのものはうっかり操作ミスで簡単に順番が入れ替わってしまうので、順番依存にせずidなどと紐づけた実装にしておきたい。
ちなみに、前項の80におよぶ長さのListは順番依存だったのでさすがにひっくり返ってしまった。
それがプロジェクト内に3か所、別々に設定されていたのだから驚きです。(本当にどうやってメンテナンスしていたのだろうか。)
続き
Discussion