【Unity 設計の話】初心者がやりがちな設計ミスにマサカリを投げてみる

はじめに

Unityでコードを書くとき、自分が普段気を付けていることを自戒の念を込めて記事としてまとめたいと思います。

かなり初歩的な話になるのでご容赦ください。

題材

今回は、１秒ごとに弾を発射する固定砲台を作ることを考えます。
シューティングゲームを作るときになどにおなじみかと思います。

これを実現するためにはいろんな方法が考えられますが、

今回はC#スクリプトを書いて弾を一定間隔で生成させることにします。

悪いコードの例

何も考えずにスクリプトを書くとこんなコードになるでしょうか。

using System.Collections;
using System.Collections.Generic;
using UnityEngine;

public class BulletGenerator : MonoBehaviour
{
     // 弾のPrefabを指定
    [SerializeField] private GameObject bulletPrefab;

    // 弾の発射ロジック
    private IEnumerator Start()
    {
        while(true)
        {
            var bullet = Instantiate(this.bulletPrefab); // 弾を生成

            var rigidbody = bullet.GetComponent<Rigidbody>();
            rigidbody.velocity = new Vector3(0f, 10f, 0f); // 弾を上に飛ばす

            yield return new WaitForSeconds(1f); // 1秒待つ
        }
    }
}

昔の自分はこのようなコードをよく書いていましたが、これは悪いコードです。

なぜ悪いコードなのか。マサカリを投げつつ理由を書き連ねていきたいと思います。

マサカリその１ : PrefabのシリアライズにGameObjectを使うのはやめよう

フィールドのタイプにGameObjectを使えばPrefabを設定することができますが、これは色々な問題を招くのでやめましょう。

問題その１: Prefabの指定ミスが起きるリスク

PrefabのタイプをGameObjectにすると、あらゆるPrefabが指定できてしまいます。

本来は弾のPrefabが入る想定なのに間違えてエネミーのPrefabを指定しまう、といったヒューマンエラーを招きます。

問題その２: コンポーネントへのアクセスが汚い

弾のPrefabにRigidbodyがアタッチされていることを想定し、「弾のRigidbodyコンポーネントにアクセスしたい」という状況を考えます。

PrefabのタイプはGameObjectなので、RigidbodyにアクセスするにはGetComponentすることになります。

var rigidbody = bullet.GetComponent<Rigidbody>();

GetComponentは重いとされているので処理負荷的にこれは良くないコードです。

問題その３：依存関係が強い

このコードでは、弾の速度を設定する際にRigidbodyを直接参照しています。

つまり、「弾の速度を設定したい」という目的に対してコンポーネントのRigidbodyが密に結合しているという関係になっています。
これは設計上良くないです。

改善案: GameObjectの代わりにクラスを指定する。

上記を踏まえて修正したものがこちらです。

using System.Collections;
using System.Collections.Generic;
using UnityEngine;

public class BulletGenerator : MonoBehaviour
{
    // 弾のPrefabを指定
    [SerializeField] private Bullet bulletPrefab;

    // Use this for initialization
    private IEnumerator Start()
    {
        while(true)
        {
            var bullet = Instantiate(this.bulletPrefab); // 弾を生成
            bullet.SetVelocity(new Vector3(0f, 10f, 0f)); // 上に飛ばす

            yield return new WaitForSeconds(1f); // 1秒待つ
        }
    }
}

GameObjectの代わりに自作クラスを指定しています。
そして、rigidbodyのvelocityに速度を代入する代わりにメソッドSetVelocityを実行して速度を指定しています。

※ 補足: Bulletクラス内部には速度を指定するメソッドpublic SetVelocity(Vector3 v) が定義されているものとします。

改善点: コードの見通しが良くなった。

速度を設定するメソッドを呼び出していることにより、「速度を設定しているんだな」ということがコードから読み取れるようになっています。

さらに、BulletGeneratorから見てBullet内部のRigidbodyの有無を意識しなくて済むようになりました。

BulletGeneratorでは「弾の速度の設定をしたい」という関心、Bulletでは「速度をどのように設定するか」という関心。
これら二つの関心をきれいに分離することができています。

これをプログラムの言葉で、「関心の分離」 と呼びます。

まとめ

・Prefabのタイプ指定にGameObjectを使うのはやめよう
・メソッドを利用して関心の分離を意識しよう

マサカリその２：数をメソッドの中に直接書くのはやめよう

コードの該当箇所を抜粋しました。

bullet.SetVelocity(new Vector3(0f, 10f, 0f)); // 上に飛ばす

yield return new WaitForSeconds(1f); // 1秒待つ

一般に、数をコードに直接指定するのは良くないとされています。(マジックナンバー)

問題その１: 数での使いまわしが難しい

例えば、以下のような状況に対処するのが難しくなってしまいます。
・弾の移動速度new Vector3(0f, 10f, 0f)を別の場所でも使いたい
・待機時間1fを別の場所でも使いたい

同じ目的の数を２か所以上に書いてしまうと、片方を変更したときにもう片方の変更を忘れてしまう、といったヒューマンエラーが発生します。

ある意味、数と目的が密に結合していると言えます。

問題その２：数の意味が分かりにくい

ソースコードを読んだ人は以下のような疑問が出てくるかと思います。
「new Vector3(0f, 10f, 0f)ってどういう値なの？」
「1fって何？」

数の直接指定は意味が分かりにくく、コードの明瞭性が落ちてしまいます。
これはヒューマンエラーを招きます。

改善案 : 定数を定義する

上記を踏まえて以下のようにコードを修正します。
読み取り専用な定数を定義し、ロジック内では定義した定数を利用しています。

using System.Collections;
using System.Collections.Generic;
using UnityEngine;

public class BulletGenerator : MonoBehaviour
{
    // 弾の移動速度
    static readonly Vector3 BULLET_VELOCITY = new Vector3(0f, 10f, 0f);

    // 弾の発射の間隔
    const float BULLET_INTERVAL = 1f;

    // 弾のPrefabを指定
    [SerializeField] private Bullet bulletPrefab;

    // 弾の発射ロジック
    private IEnumerator Start()
    {
        while(true)
        {
            var bullet = Instantiate(this.bulletPrefab); // 弾を生成

            bullet.SetVelocity(BULLET_VELOCITY); // 速度設定

            yield return new WaitForSeconds(BULLET_INTERVAL); // 待つ
        }
    }
}

定数を利用することで、その数が何を意味するのかが見えやすくなりました。
また、定数の使いまわしもできるようになりました。

まとめ

・コード中に数を直接記述するのは避けて定数を定義しよう。

その他の懸念事項

ほかにも以下のようなことが考えられると思います。

・BulletGeneratorには弾の発射ロジックだけ持たせておいて、弾の発射の実行は別のクラスにさせるべきでは？（単一責任原則）
・弾のPrefabはBulletGeneratorに持たせて良いのか？　Prefabを管理するクラスを作ってそこで管理するべきなのでは？
・定数はBulletGeneratorに直接持たせて良いのか？　定数を管理するクラスを作ってそこで管理するべきなのでは？
・弾の一定間隔の発射はコルーチンを使わずにUniRxを使うべきなのでは？

ここらへんはプロジェクトによって正解が変わるかと思います。
銀の弾丸は無い

補足: 環境について

Windows 10
Unity2018.1.0b7