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

Unityの初心者向けのサイトや書籍でよくやってしまう悪手が幾つかあります。気付かない原因は、

スケールが小さいゲームだと処理落ちも最初はほとんどなくて放置していること、全体が小さいので自分で管理しているだけだと気にならない
ネットや書籍に書いてある安易な方法が、小さなサンプルでのみ有効でスケールを大きく作ると支障になる

という場合です。その後、スケールが大きくなったり多人数で共同作業すると失敗することがよくあります。そうしたパターンを並べてみます。

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

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

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

アンチパターンの例

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

[パターン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()で遅延実行していることになります。
テストして確認すると、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 );
}

この処理は

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

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

private CautionDisplayManager cautionDisplayManager;

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

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

[参考]

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

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

1	2	3	4
player	monster	citizen	animal
人型の主人公	怪物	街の人	街の動物

それぞれの処理をまとめておきたいと考えてひとつのコンポーネントを作ったとします。

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で切り替え汎用性を高めようという設計です。しかしながら、アタッチの仕方を変えればこの判定は要りません、毎フレームの無駄な判定の典型です。このコンポーネントの形が有効になる可能性があるとすれば、敵味方が一秒のうちに数十回など頻繁に入れ替わって毎フレーム変化する、という場合ですが、果たしてそういうゲームでしょうか。
この無駄を省く対策方法はいくつかありますが、上記でやったフラグの変更を残してActionやDelegateで処理する、という方法もありますが、別の方法を探るとした場合、コンポーネントを分けることで対応したとしましょう。

まずは仕様を確認

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

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

全タイプの処理
Player
Monster
人型

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

方法1)独立した`Component`にする

それぞれでMonoBehaviourから派生したclassを作ります。

Character.cs

public class Character : MonoBehaviour
{
    void Update()
    {
        /*
        全タイプの処理がここにある
        */
        Debug.Log("Update on Character");
    }
}

Player側はCharacterの全タイプの処理を必須にアタッチしたいので[RequireComponent(typeof(Character))]によって確実にアタッチさせます。

Player.cs

[RequireComponent(typeof(Character))]
public class Player : MonoBehaviour
{
    // Update is called once per frame
    void Update()
    {
        // Playerの処理
        Debug.Log("Update on Player");
    }
}

他のclassも同様に作れます。ただし、ここで問題点があります。Updateの実行順序が制御できない点です。

試しにこの状態で動かすと、実行順番が以下になります。

この状態は容易に変えられないことを覚えておきましょう。もし、元の形のように共通の処理をやってからPlayer個別の処理を実行したい場合は、以下のように検討します。

Updateが必要か見直して他に変える
Character側のUpdateを別のメソッドに変えて廃止し、Palyerから呼び出す

Updateは汎用性が高いですが、色々な問題を孕みやすいのでできるだけ排除した方が得策になります。

方法2)派生した`Component`にする

処理が内包されているので、上記のCharacterから派生したPlayerExを作ってみます。

PlayerEx.cs

public class PlayerEx : Character
{
    // Update is called once per frame
    void Update()
    {
        // Playerの処理
        Debug.Log("Update on Player");
    }
}

これのみをアタッチして実行してみます。

そうすると、PlayerExのUpdateのみが実行されます。
この場合はCharacter側でのUpdateが無効になるので、Character側のメソッドをUpdateではないpublicかprotectedの別のメソッドにし、それを呼び出す形にすることで対処は可能ですが、前述の個別でコンポーネントを持たせて対応するのと変わらない状況になります。
いずれにせよ、仕様で検討が必要なのは、

Updateが複数ある状況は注意が必要

ということです。それでは、初めのUpdate内で判定していたものが好いのか、というとそうではありません。CPU資源をできるだけ消費しないことは、常に意識が必要です。特に判定文はCPU実行のパイプラインにも影響するので、できるだけ排除した方が好いことになります。例えば、時間経過で処理するのにDeltaTimeを用いるのもCoroutineなど他の方法が色々、あります。入力処理はInputSystemとOnMoveがあればUpdate()不要で無駄が減ります。
実行順番を制御したい場合、以下も参考になるので見ておきましょう。

[Unity公式の情報]

方法3)処理判定を`Awake`で一度だけにして`Action`で処理を構築する

コードが長いですが、元の処理をそのまま再現できる方法がこれになります。

CharacterControllerEx.cs

public class CharacterControllerEx : MonoBehaviour
{
    public enum CharacterType {　// キャラクターのタイプ
        player, 
        monster, 
        citizen , 
        animal
    }
    private Action charactorAction=null;
    private CharacterType _characterType;
    public CharacterType type {
        get => _characterType;
        set
        {
            _characterType = value;
            SetAction();
        }
    }
    [SerializeField]CharacterType defaultType = CharacterType.player;
    private void AllAction()
    {
        //　全プレイヤーの処理
        Debug.Log("Action All Character");
    }

    private void PlayerAction()
    {
        //　全プレイヤーの処理
        Debug.Log("Action Player");
    }
    private void MonsterAction()
    {
        //　全プレイヤーの処理
        Debug.Log("Action monster");
    }
    private void HumanAction()
    {
        //　全プレイヤーの処理
        Debug.Log("Action Human");
    }
    private void SetAction()
    {
        charactorAction = AllAction;
        if (type == CharacterType.player)
        {
            //　Playerの処理
            charactorAction += PlayerAction;
        }
        else if (type == CharacterType.monster)
        {
            //　Monsterの処理
            charactorAction += MonsterAction;
        }
        if (type == CharacterType.citizen || type == CharacterType.player)
        {
            // 人型の処理
            charactorAction += HumanAction;
        }
    }

    private void Awake()
    {
        type = defaultType;
    }
    // Update is called once per frame
    void Update()
    {
        charactorAction();
    }
}

処理をそれぞれのブロックごとをメソッド化し、Actionに必要な処理を登録します。こうすることでロジックが簡略化します。もっと進めるとDictionaryでActionにする方法が可能です。

 Dictionary<CharacterType,Action> MethodTable = new Dictionary<CharacterType,Action>();

このDictionaryにCharacterType毎のMethodを登録しておきます。そしてUpdateで実行するActionにコピーします。汎用性が一番高いのはこれですが、大分、コードが長くなります。後々に利用する可能性があるならあっても好い、という程度でしょうか。

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

例えばゲームオーバーの判定を複数個所で行っている、という場合です。Scriptが一つでも複数にアタッチしていて同じ判定をしていると無駄が生じます、適宜、中身を見直しましょう。

[パターン5]FindやGetComponentを多用する

これの難点は以下になります。

何処のオブジェクトから何を操作しているのか判らなくなると、バグがあった場合に切り分けができず解決を困難にする
文字列比較をしているとその分のタイプミスや無駄な動作から逃れられない

オブジェクトの参照は一方向にして、出来るだけ他のコンポーネントの参照を控える形にしておくと、デバッグやメンテナンスが飛躍的に容易くなるのでお勧めします。文字列もtagなどで用いますが、レイヤを駆使して利用を控えるなどが効率化につながります。
プログラミングパターンを学ぶとよいでしょう。

まとめ

Unityは容易くゲームが作れることを目標にしているため、Update()で簡単にゲームを組み立てるなど、そのノリに乗っかりすぎてる場合、簡単にゲームが作れるけど、求めていたものが作れる道をぼかしてしまいます。
実装が進むとどんどんゲームらしくなってきて面白いので見直さずに手を休めずに作り続けた結果、増築を繰り返した建造物のように全体がアンバランスで、手を加えるのがどうしたら好いか分らなくなりがちです。そうなるともう作りたくなくなって、また最初から作ろう、ということにも繋がり、これがループするだけだとまた同じことを繰り返して半端な制作物を量産しがちです。
きちんと設計しておいた方が後から楽で、更に再利用できる部分が増えていくので、また新しいものに取り掛かっても無駄が少なくなります。
そうしたことを考えて製作に臨みましょう、手を動かしただけ、後に指数関数的に能力が拡張できます。