【開発日記】昔書いたUnityのソースコードがひどかったので晒して突っ込んでみた件

というわけで、かなりネタに飢えているゴマちゃんフロンティアです。マイキャラの作り直しを始めたが最後、どうしても妥協できずに再修正を繰り返しています。
キャラクターはオリジナルゲームにおいて強烈な個性を表現できる部分なので、悩ましいところではあります。そこを拘らないのであれば、アセットストアから適当にチョイスしたものと変わらなそうなのでなおさらです。

そんな経緯から、今回のネタは「過去の自分を見つめ直す」と称して、昔作ったUnityプロジェクトのソースコードを眺めてみました。すると、あまりの酷さにネタとして取り挙げることができるレベルだったので紹介します！

ソースコード

特にひどかったのはプレイヤークラスのソースコードです。ちょっと長いですが、とりあえず最初に載せておきます。

using UnityEngine;
using System.Collections;

[System.Serializable]
public class Player : MonoBehaviour {

    //Object & Compornent
    public CharacterController charControl;
    public CharacterMotor charMotor;
    public PlatformInputController platForm;
    public Transform weapon;
    public ParticleSystem weaponParticle, powerMaxParticle, onLimitParticle, OnSpownParticle, ChargeParticle, invincible;
    public GameObject bullet, dastBullet, OnChargeing, finalSord;
    public GameObject []hitObject;
    public Animator animator;
    public AnimatorStateInfo stateInfo;
    protected GlobalParameter gp;
    private StageManager manager;

    //parameter
    public float life, limit, maxLife, maxLimit, powerLevel, charge, maxcharge;
    public int score;
    public Vector3 []hitOffset;
    public Texture2D charTex;
    public AudioClip[] commonActions;
    protected float moveCount, dastShootCount;
    protected bool invincibility, isAreaMeter;
    private int extendScore, recoveScore;

    //Textuer
    public GameObject headObject, accessory;
    public Texture2D []faceTex;
    public Material secondColor;

    // Use this for initialization
    protected virtual void Start () {
        charControl = GetComponent<CharacterController>();
        charMotor = GetComponent<CharacterMotor>();
        platForm = GetComponent<PlatformInputController>();
        animator = GetComponent<Animator>();

        gp = GameUtility.GetGlobalParameter();
        manager = GameUtility.GetStageManager();

        //parameter Set
        maxLife = life;
        maxLimit = 100;
        charge = 0;
        maxcharge = 100;
        powerLevel = 1;
        score = 0;
        dastShootCount = 0;

        //ControllerCheck
        if(networkView.isMine == false){
            Destroy(platForm);
            Destroy(charMotor);
        //    Destroy(charControl);
        }

        if(manager.player.name == "Rabbit")
            gp.myTexture = 1;
        else if(manager.player.name == "Pengin")
            gp.myTexture = 2;
        else if(manager.player.name == "Seal")
            gp.myTexture = 3;
        else if(manager.player.name == "Kangaroo")
            gp.myTexture = 4;

        if(networkView.isMine){
            manager.networkView.RPC("InitializePlayer", RPCMode.AllBuffered, life, gp.myNumber, gp.myTexture);
            networkView.RPC("ColorChange", RPCMode.AllBuffered, gp.myColor);
        }
    }


    protected virtual void Update () {
        if(networkView.isMine){
            //DebugKey
            if(Input.GetKeyDown(KeyCode.L))
                limit += 100;
            if(Input.GetKeyDown(KeyCode.P))
                charge += 120;

            //weaponParticle
            if(powerLevel == 2)
                networkView.RPC("WeaponParticle", RPCMode.AllBuffered, 2.0f);
            else if(powerLevel == 3)
                networkView.RPC("WeaponParticle", RPCMode.AllBuffered, 3.0f);
            else if(powerLevel == 9)
                networkView.RPC("WeaponParticle", RPCMode.AllBuffered, 9f);

            //chargeUp
            if(charge > 100){
                if(powerLevel < 3){
                    powerLevel++;
                    charge = 0;
                }
                else if(powerLevel >= 3){
                    powerLevel = 9;
                    charge = 100;
                }
            }
            //PowerLevel 9
            if(powerLevel >= 9){
                powerLevel = 9;
                charge -= Time.deltaTime * 10;

                if(charge < 0){
                    powerLevel = 3;
                    charge = 50;
                }
            }

            //MoveCount
            if(Input.GetMouseButton(0) || Input.GetButton("Attack"))
                moveCount++;
            else if(Input.GetMouseButtonUp(0) || Input.GetButtonUp("Attack"))
                moveCount = 0;

            //Animator
            if(animator){
                stateInfo = animator.GetCurrentAnimatorStateInfo(0);

                //Dash
                if(stateInfo.IsName("Base Layer.Move") && moveCount >= 20){
                    charMotor.movement.maxForwardSpeed = 30;
                    networkView.RPC("motionChange", RPCMode.AllBuffered, "Dash", true);
                }
                //Move
                else if(charMotor.GetDirection().magnitude > 0.1 && charMotor.IsGrounded()){
                    if(stateInfo.IsName("Base Layer.Move") == false
                    && stateInfo.IsName("Base Layer.Dash") == false)
                        networkView.RPC("motionChange", RPCMode.AllBuffered, "Move", true);
                }

                if(charMotor.GetDirection().magnitude < 1 && charMotor.IsGrounded()){
                    if(stateInfo.IsName("Base Layer.Move"))
                        networkView.RPC("motionChange", RPCMode.AllBuffered, "Move", false);
                    if(stateInfo.IsName("Base Layer.Dash"))
                        networkView.RPC("motionChange", RPCMode.AllBuffered, "Dash", false);
                    moveCount = 0;
                    charMotor.movement.maxForwardSpeed = 20;
                }



                //Jump
                if(stateInfo.IsName("Base Layer.Idle") || stateInfo.IsName("Base Layer.Move") || stateInfo.IsName("Base Layer.Dash")){
                    if(!charMotor.IsGrounded()){
                        moveCount = 0;
                        networkView.RPC("motionChange", RPCMode.AllBuffered, "Move", false);
                        networkView.RPC("motionChange", RPCMode.AllBuffered, "Jump", true);
                    }
                }
                else if(stateInfo.IsName("Base Layer.Jump") && charMotor.IsGrounded()){
                    networkView.RPC("motionChange", RPCMode.AllBuffered, "Jump", false);
                }

                //Rolling
                if(Input.GetMouseButtonDown(2) || Input.GetButtonDown("Roll")){
                    if(!stateInfo.IsName("Base Layer.Roll") && !stateInfo.IsName("Attack.LimitBreak1") && !stateInfo.IsName("Attack.LimitBreak2")){
                        networkView.RPC("motionChange", RPCMode.AllBuffered, "Roll", true);
                        StartCoroutine(ResetInvincibility(1f));
                    }
                }
                else if(stateInfo.IsName("Base Layer.Roll")){
                    invincibility = true;
                    charControl.Move((transform.forward  * Time.deltaTime) * 30);
                    networkView.RPC("motionChange", RPCMode.AllBuffered, "Roll", false);
                }

                //Reset   
                if(stateInfo.IsName("Base Layer.Idle") && animator.GetBool("DownRecover"))
                    networkView.RPC("motionChange", RPCMode.AllBuffered, "DownRecover", false);
                if(stateInfo.IsName("Base Layer.Damage"))
                    networkView.RPC("motionChange", RPCMode.AllBuffered, "Damage", false);

                if(life < 0)
                    life = 0;
                if(life > maxLife)
                    life = maxLife;
                if(limit > maxLimit)
                    limit = maxLimit;

            }
        }
    }


    //AttackInstantiate
    [RPC]
    public virtual void AttackInstantiate(int obj, int offset, NetworkViewID viewID){
        GameObject weaponParent;
        Vector3 tmp;

        weaponParent = Instantiate(hitObject[obj], weapon.position, weapon.rotation) as GameObject;
        weaponParent.transform.parent = weapon;
        tmp = weaponParent.transform.localPosition;
        tmp += hitOffset[offset];
        weaponParent.transform.localPosition = tmp;
        weaponParent.GetComponent<Hit>().pc = this;

        weaponParent.networkView.viewID = viewID;

        if(hitObject[obj].tag == "ChargeAttack")
            ChargeParticle.Play();
        if(obj == 4)
            weaponParent.transform.Rotate(0f, 0f, 270);

        audio.PlayOneShot(commonActions[1]);
    }



    //ResetAttack
    public IEnumerator AttackReset(string name){
        yield return new WaitForSeconds(1f);
        networkView.RPC("motionChange", RPCMode.AllBuffered, name, false);
    }

    //Item
    public virtual void GetItem(string tag){
        //parameterUp
        if(tag == "lifeUp"){
            if(maxLife < 200)
                maxLife += 10;
            life = maxLife;

            if(networkView.isMine)
                manager.networkView.RPC("SetPlayerLife", RPCMode.AllBuffered, life, gp.myNumber);
        }
        if(tag == "limitUp"){
            if(maxLimit < 200)
                maxLimit += 10;
            limit = maxLimit;
        }
        if(tag == "powerUp"){
            if(powerLevel < 3)
                powerLevel++;
            else if(powerLevel >= 3){
                powerLevel = 9;
                charge = 100;
            }
        }

        //fluits
        if(tag == "banana"){
            GetScore(500);
        }
        if(tag == "grape"){
            GetScore(1500);
        }
        if(tag == "apple"){
            GetScore(3000);
        }

        //finalSord
        if(tag == "finalSord"){
            GameObject finalParent;
            Vector3 tmp;

            finalParent = Instantiate(finalSord, weapon.position, weapon.rotation) as GameObject;
            finalParent.transform.Rotate(0f, 0f, 270);
            finalParent.transform.parent = weapon;
            tmp = finalParent.transform.localPosition;
            tmp += hitOffset[1];
            finalParent.transform.localPosition = tmp;
            finalParent.GetComponent<FinalSord>().pc = this;   
        }

        audio.PlayOneShot(commonActions[0]);
    }

    //weaponParticle
    [RPC]
    public virtual void WeaponParticle(float powerLevel){
        if(powerLevel == 2){
            weaponParticle.Play();
            weaponParticle.emissionRate = 300;
        }
        else if(powerLevel == 3){
            weaponParticle.Play();
            weaponParticle.emissionRate = 1000;
            weaponParticle.startSize = 1.5f;
        }
        else if(powerLevel == 9){
            powerMaxParticle.Play();   
        }
    }

    //motionChange
    [RPC]
    public void motionChange(string state, bool flag){
        if(animator)
            animator.SetBool(state, flag);
    }


    //Alive
    public bool IsAlive(){
        if(life > 0)
            return true;
        else
            return false;
    }

    //Score
    void GetScore(int s){
        for(var i=0; i<s; i++){
            score++;
            recoveScore++;
            extendScore++;

            if(recoveScore >= 500){
                life += 10;
                recoveScore = 0;
            }

            if(extendScore >= 5000){
                if(maxLife < 200)
                    maxLife += 10;
                if(maxLimit < 200)
                    maxLimit += 10;
                extendScore = 0;
            }
        }
    }

    //Recover
    public void LifeRecover(){
        life = maxLife;
        if(networkView.isMine)
            manager.networkView.RPC("SetPlayerLife", RPCMode.AllBuffered, 100f, gp.myNumber);

        //Reset
        charge = 0;
        score = score/2;

        if(networkView.isMine)
            Network.Instantiate(OnSpownParticle, transform.position, Quaternion.identity, 0);
    }

    //Damage
    [RPC]
    public void Damage(float power){
        if(invincibility == false){
            invincibility = true;
            invincible.Play();

            life -= power;
            if(life < 0)
                life = 0;

            if(networkView.isMine)
                manager.networkView.RPC("SetPlayerLife", RPCMode.AllBuffered, life, gp.myNumber);

            //SuperArmore
            if(powerLevel != 9)
                networkView.RPC("motionChange", RPCMode.AllBuffered, "Damage", true);
            headObject.renderer.materials[1].SetTexture("_MainTex", faceTex[1]);
            if(life > 1){
                StartCoroutine(TextureReset(0.4f));
                StartCoroutine("ResetInvincibility", 1f);
            }
            else{
                StartCoroutine(TextureReset(1.5f));
                StartCoroutine("ResetInvincibility", 3f);
            }

            networkView.RPC("motionChange", RPCMode.AllBuffered, "Attack1", false);
            networkView.RPC("motionChange", RPCMode.AllBuffered, "Attack2", false);
            networkView.RPC("motionChange", RPCMode.AllBuffered, "Attack3", false);
            networkView.RPC("motionChange", RPCMode.AllBuffered, "DastAttack", false);
            networkView.RPC("motionChange", RPCMode.AllBuffered, "Move", false);
            networkView.RPC("motionChange", RPCMode.AllBuffered, "ChargeAttack", false);
        }
    }

    public IEnumerator ResetInvincibility(float time){
        yield return new WaitForSeconds(time);
        invincibility = false;
    }

    //Texture
    protected IEnumerator TextureReset(float time){
        yield return new WaitForSeconds(time);
        headObject.renderer.materials[1].SetTexture("_MainTex", faceTex[0]);
    }   

    //ColorSelect
    [RPC]
    public virtual void ColorChange(int color){
        if(color == 1)
            accessory.renderer.materials = new Material[]{secondColor};
    }

    //Colled LimitStay
    [RPC]
    public void CallLimitStay(NetworkViewID viewID){
        StartCoroutine("LimitStay", viewID);
    }
}

具体的にダメな部分

自分への復習の意味も込めて、片っ端から突っ込んでみます。

ハードコーディング

かなりハードです。ガッチガチです。

if(manager.player.name == "Rabbit")
    gp.myTexture = 1;
else if(manager.player.name == "Pengin")
    gp.myTexture = 2;
else if(manager.player.name == "Seal")
    gp.myTexture = 3;
else if(manager.player.name == "Kangaroo")
    gp.myTexture = 4;

初っ端からこんな感じなので、悪い意味で期待が持てます。なかなか香ばしいソースコードです。

・プレイヤーキャラが増えたときにelse ifを追加しなければならない
・キャラクター名が変わったときに条件文の修正が必要
・gp.myTextureに素の番号を入れる意味が不明
・ペンギンのスペルが違う (普通は「Penguin」)

「そもそもgpってなんだ？」と思いきや、GameGlobalParameterなるクラスのようです。そこにmyTextureなるフィールドがある意味も、PlayerクラスのStart()から代入する意味も分かりません。

マジックナンバー＆マジックストリング

数値or文字列の意図が分かりにくいものが多数あります。上のハードコーディングの例でいえば、数値の「1～4」が何を示しているのかさっぱり分かりません。
クラス全体がこんな調子なので、ソースコード単体で追いかけても処理が分かりにくいです。

(悪い意味で)スーパーなクラス

そもそも1クラスの持つ機能が多すぎます。悪い意味で「スーパークラス」です。

例えば関数でGetItem()なるものがあります。

//fluits
if(tag == "banana"){
    GetScore(500);
}
if(tag == "grape"){
    GetScore(1500);
}
if(tag == "apple"){
    GetScore(3000);
}

「何故Playerクラスで取得したアイテムの選別をしているのか」という話です！普通はバナナとかブドウに相応するクラスを用意し、そこからPlayer.addScore()(またはプロパティ)のように実装しますよね。
ハードコーディングの話と被りますが、「フルーツ増えたらelse ifが(ry」「フルーツ名変わったら(ry」「スコア加算値が変わったら(ry」という問題もあります。

やけにオープンなメンバ

メンバのアクセス修飾子がほぼpublicです。まさにカプセル化()状態です。
まあ開発者が自分1人なのでカプセル化もそこまで意識しなくてよいかと思いますが、後々見直したときにpublicだらけだとやはり厳しいです。せめてクラス内でしか使用しない関数くらいprivateにしてほしいところですね。

変数のスコープ

クラス内で使うもの変数のほとんどがフィールドで宣言されています。どうやら「変数のスコープ」という概念もなかったようです。もちろんプロパティなんて素敵なものも使っておりません。

//Object & Compornent
public CharacterController charControl;
public CharacterMotor charMotor;
public PlatformInputController platForm;
public Transform weapon;
public ParticleSystem weaponParticle, powerMaxParticle, onLimitParticle, OnSpownParticle, ChargeParticle, invincible;
public GameObject bullet, dastBullet, OnChargeing, finalSord;
public GameObject []hitObject;
public Animator animator;
public AnimatorStateInfo stateInfo;
protected GlobalParameter gp;
private StageManager manager;

//parameter
public float life, limit, maxLife, maxLimit, powerLevel, charge, maxcharge;
public int score;
public Vector3 []hitOffset;
public Texture2D charTex;
public AudioClip[] commonActions;
protected float moveCount, dastShootCount;
protected bool invincibility, isAreaMeter;
private int extendScore, recoveScore;

//Textuer
public GameObject headObject, accessory;
public Texture2D []faceTex;
public Material secondColor;

何やってるか分からない関数群

命名規則が歪な上、まともにコメントも書かれていません。フィールド変数もアレなので入力補完的な意味でも厳しいです。

ダメではないが直したい部分

コードフォーマット

好みの問題ではありますが、ifやコメントの後に空白がないのが気になります。

//fluits
if(tag == "banana"){
    GetScore(500);
}
if(tag == "grape"){
    GetScore(1500);
}
if(tag == "apple"){
    GetScore(3000);
}

これをちょっと直して、

// fluits
if (tag == "banana") {
    GetScore(500);
}
if (tag == "grape") {
    GetScore(1500);
}
if (tag == "apple") {
    GetScore(3000);
}

ぎっしり書くより多少ゆとりを持たせて書いた方が見やすいです。PHPのPSR-4もこんな感じですよね。

条件文の書き方

例えばこちら。

public bool IsAlive(){
    if(life > 0)
        return true;
    else
        return false;
}

「三項演算子」使いません？

public bool IsAlive(){
    return life > 0 ? true : false;
}

うん、こっちの方がスマートかと思いますよ。

あとがき

そんなわけで、昔の自分のソースコードを晒しあげてみました！
初心の頃は「とりあえず動けばいい」と考えてしまいがちです。自分も後々のことを考えずに書きまくったり、ネット上のサンプルを理解せずにコピペしたりしていました。
特にフレームワークなどは内部処理を意識せずに使ってしまいますが、頑張って中身を追ってみると意外なことが分かったりします。まあフレームワーク自体がそういうものですが、お世話になっているプログラムがある場合は一度追ってみるのも面白いかもしれません。

ちなみに今回晒したソースコード、今の自分風にリファクタしようとも考えましたが、手を入れるべき部分が多すぎるためやめておきました。まともにやったらどれだけ時間が掛かるか分かったもんじゃないですね。