Unityでリファクタリングことはじめ、サンプルで見かける冗長コードのアンチパターン

ダメだといわれるコードを書いてしまう要因

Unityの初心者向けのサイトや書籍でよくやってしまう悪手が幾つかあります。気付かない原因は、スケールが小さいゲームだと処理落ちも最初はほとんどなくて放置していること、全体が小さいので自分で管理しているだけだと気にならない、という場合です。その後、スケールが大きくなったり多人数で共同作業すると失敗することがあります。そうしたパターンを並べてみます。

何に気を付けるべきなのか

基本的に何を気にすべきかは決まっています。大まかには以下を今回は取り上げます。

1. 処理が冗長になっていないか
   同じ処理を一度でいいのに繰り返しているところ。これは目の前のコードでアルゴリズム的に無駄な場合と、フレームを跨いで同じ処理を繰り替えず場合、同じ処理を多くのインスタンスでやっている場合も当てはまります。
2. 大規模になってから設計が耐えられるか
   適当に何処からでも他のGameObjectを参照することが可能な設計がUnityの基本で、そこから「触らない方が好い」ものも触れてしまいます、そのために誰かが意図しないでどこかの変数を書き換える、ということが生じます。これをできるだけ避けた方が、意図しない動作をしたときに原因を突き止めて修正がしやすくなります。

アンチパターンの例

ここからは、実際によく見かける、初心者がサンプルから悪手を学んでしまうパターンを挙げてみます。

処理が冗長になる、無駄にCPUを喰う例

[パターン1]Update()に色々入れる

Update()に入っている処理は、フレームをまたぐと無条件で実行される無限ループの中に書いたのと同じです。それに気づかないと無駄を生みます。
例えば、何かのフラグがtrueになってるとき"CAUTION"と出すGameObjectを有効にする、という処理を書いたとします。

public bool cautionFlag=false;
public GameObject cautionDisplay;
void Update(){
    if( cautionFlag )
    {
        cautionDisplay.SetActive( true );
    }
    else
    {
        cautionDisplay.SetActive( false );
    }
}

この場合、何のUpdate()でやってるかということもありますが、Flagを毎フレームで判断します、もし毎フレームの状況が刻々と変化しないなら、無駄です。

まず、単純化すると以下に書き直せます。

public bool cautionFlag=false;
public GameObject cautionDisplay;
void Update(){
    cautionDisplay.SetActive( cautionFlag );
}

ifは排除できましたが、これでも「毎フレームの状況が刻々と変化するか」によって、状況は変わります。この場合、cautionFlagを引数にしてSetActiveを呼ぶので、アセンブラでいうと単純にPUSHとCALL、POP、RETURNは最低限、実行されます（インライン展開が無い場合）。既にtrueでもまたtrueでというように、同じ値の状態でも実行されるので、結果的に無駄です。
この場合では、cautionFlagを書き換えている部分がこのコード上では無いので、何処か他で書き換えないと制御できません。そのフラグを書き換えるタイミングが、この場合は明確にあればその時々でやればいい処理で、それをあえてUpdate()で遅延実行していることになります。この処理は、単純化するとこのように書くことができます。

public bool cautionFlag=false;
public GameObject cautionDisplay;
void Update(){
    cautionDisplay.SetActive( cautionFlag );
}

テストして確認すると、cautionFlagがtrueになるフレームの次のフレームで表示されるのが確認できます。
この形のままで、無駄に状態が変わらないときに処理しなくする方法は以下があります。

    public GameObject cautionDisplay;
    private bool cautionFlag = false;
    public bool CautionFlag
    {
        get => cautionFlag;
        set
        {
            if (value != cautionFlag)
            {
                cautionFlag = value;
                cautionDisplay.SetActive(cautionFlag);
            }
        }
    }
    private void Start()
    {
        cautionDisplay.SetActive(cautionFlag);
    }

setでCautionFlagに何か代入されたときに、値が書き換わっているかを確認しています。書き換えられた時にはSetActiveを実行し、状態を変えます。
これをやらなくても、おそらくGameObjectの内部で同様の処理はしていると想像できますが、呼び出す手間が無駄であり、そして毎フレームすることが無駄なので、これで解消できます。遅延実行もないのでそのフレームの中で処理が行われ、遅れて変化することもありません。
プログラムの処理はCPUとGPUで１フレームを作ることの繰り返しなので、基本的にフレーム毎のループ処理で、ゲームエンジンを用いない場合はC++であればmain()などのエントリポイントから初期化後に必ずループする部分があります。つまり、すべてをUpddate()に置けば理論上はすべて実装できて動きます。そうした理由で、実装することだけを目標にすると、こうした無駄な処理に気付かないことがあります。

---

[パターン2]値が変わらないものを再び探す

以下のコードがあったとします。

[RequireComponent(typeof(CautionDisplayManager))]

void ChangeMessage(string message){
    GetComponent<CautionDisplayManager>().SetText( message );
}

この処理は

1. GameObject.GetComponentでを用いて型に合うComponentを探す
2. その中の.SetTextというメソッドを実行する

という２段階の処理が実行されており、前段階はComponentの差し替えが無い場合は同一の参照を返します。勿論、差し替えられる可能性がある場合は使えませんが、差し替えられることがバグでしかない場合は無視できます。そうした場合は以下に書き換えた方が得策になります。

private CautionDisplayManager cautionDisplayManager;

void Awake(){
     cautionDisplayManager = GetComponent<CautionDisplayManager>();
}
void ChangeMessage(string message){
    cautionDisplayManager.SetText( message );
}

Update()内で実行している場合があったら、確実に見直しましょう。他の参照を得るFind等も同様です。こうしてキャッシュしておくことで冗長な処理がなくなり、処理落ちの一因を排除できます。こうした処理や変数の生成も相まって、毎回、繰り返す必要があるかを常に考えましょう。確実に一意の参照しかない場合は、もしかするとstaticを用いたり、分けたclassをひとつにまとめた方が好い、という場合もあります。

---

[パターン3]何にアタッチされているかをコンポーネント側で判断する

コンポーネントの汎用性を高めるため、色々なGameObjectにアタッチできるようにしていくうちに、何かを判定して振る舞いをかえるように考えて、以下のようなclassを作ったとします。

public class CharacterController : MonoBehaviour
{
    public enum CharacterType { // キャラクターのタイプ
        player, 
        monster, 
        citizen , 
        animal
    }
    public CharacterType type;

    // Update is called once per frame
    void Update()
    {
        /*
        全タイプの処理がここにある
        */
        //タイプで分かれる処理
        if (type == CharacterType.player)
        {
            //　Playerの処理
        }
        else if (type == CharacterType.monster) {
            //　Monsterの処理
        }
        if (type == CharacterType.citizen|| type== CharacterType.player)
        {
            // 人型の処理
        }
    }
}

これをゲーム内のGameObjectにそれぞれアタッチして、CharacterTypeで切り替え汎用性を高めようという設計です。しかしながら、アタッチの仕方を変えればこの判定は要りません、毎フレームの無駄な判定の典型です。
対策方法はいくつかありますが、コンポーネントを分けることで対応します。

まずは仕様を確認

type	player	monster	citizen	animal
全タイプの処理	◯	◯	◯	◯
playerの処理	◯
Monsterの処理		◯
人型の処理	◯		◯

処理がこのように分かれているので、この場合で必要なのは、以下のComponentになる。

• 全タイプの処理
• Player
• Monster
• 人型

これらをそれぞれ個別でComponentにする。どのように個別で用意するかは、以下の方法がある。

独立したComponentにする

---

[パターン4]同じ判断を複数のインスタンスで行う

前後を見ないのに配列の参照を使う

これはよくあるのがInputでゲームパッドを配列で持っているときに、どれがどこの操作をしているかをさんしょうすることです。

設計上であとから困る例

FindやGetComponentを多用する