About

個人開発のゲームにてswitch文が増え、コードがヤバくなってしまい、リファクタリングをしようと考えた今日この頃。

もう1度OCPについて考える機会だと思い、この記事を書いています。

↓前回のOCP紹介記事

qemelly.hatenablog.com

OCPについて再整理

2度目なのでサラッと。

OCPとは、「クラスは拡張に対して開かれており、修正や変更に対しては閉じていなければならない」という原則です。

これは以下のような目的を含んでいます。

クラスの機能を拡張する際、既存のコードに手を付ける行為を最小限にする
依存関係を最小限化し、コードの意図を分かりやすくする

OCPに違反してるコードってどんなの？

switch文

分かりやすさのために主語がデカいです。

やむを得ずswitchを使うこともありますが、switchを使う際はOCPに違反していないかを充分考える必要があります。

闇雲にswitch文を書くと、パターンを修正した際にすべてのswitch文を修正する必要に駆られてしまいます。

前の記事で紹介した図形の面積問題をもう一度取り上げてみます。

using UnityEngine;

namespace DesignPatterns.OCP
{ 
    public class AreaCalculator 
    {
        public float GetRectangleArea(Rectangle rectangle)
        {
            return rectangle.Width * rectangle.Height;
        }

        public float GetCircleArea(Circle circle)
        {
            return circle.Radius * circle.Radius * Mathf.PI;
        }
    }

    // AreaCalculatorは以下とほぼ同じ意味。
    public class AreaCalculateManager
    {
        public float GetArea<T>(T obj)
        {
            return obj switch
            {
                Rectangle rectangle => rectangle.Width * rectangle.Height,
                Circle circle => circle.Radius * circle.Radius * Mathf.PI,
                _ => 0f
            };
        }
    }

    public class Circle
    {
        public float Radius { get; set; }
    }

    public class Rectangle
    {
        public float Width { get; set; }
        public float Height { get; set; }
    }
}

前記事にAreaCalculateManagerを追加してみました。このクラスはAreaCalculatorとほぼ同じようなクラスになっています。GetArea()を呼び、その引数のクラスをパターンマッチングによるswitch式で分岐しています。このようなコードは一見直感的なのですが、開発を進めていくうちに破壊的なコードに変貌します。

AreaCalculateManagerは、むやみやたらなswitch文が危険であることを説明するためのものです。switch文を書くときはもっといいコードがないかを考えるのは結構大事な気がしています。

なんちゃらCalculatorとかなんちゃらProcessorとか

個人的にOCP違反のクラス設計に出てきがちなクラス。個々のクラスが処理できておくべきことを外部に移譲してしまうと「なんちゃらCalculatorとかなんちゃらProcessorとか」ができてしまいます。結果的にOCPに違反している可能性が高いです。

関数的対応をするモノ

違反例にて後述します。

実際の違反例

音ゲー制作にて過去に自分が違反した例を紹介。判定結果に応じてスコアを加算する部分です。

using System;
using UnityEngine;

namespace Scores
{
    /// <summary>
    /// 判定の結果
    /// </summary>
    public class JudgementResult
    {
        public readonly JudgementResultType Type; // enum
        public readonly int LagMs; // 判定からのズレ(ms)

        public JudgementResult(JudgementResultType type, int lagMs)
        {
            Type = type;
            LagMs = lagMs;
        }

        // ...
    }
}

namespace Scores
{
    public class ScoreModel : MonoBehaviour
    {
        // ... 

        private void AddScore(JudgementResultType type)
        {
            var addition = type switch
            {
                JudgementResultType.Perfect => 15,
                JudgementResultType.Great => 10,
                JudgementResultType.Good => 5,
                JudgementResultType.Bad => 1,
                JudgementResultType.Miss => 0,
                _ => 0
            };

            _score.Value += addition;
        }
    }
}

がっつりswitch文を使ってますが、これはOCP違反でしょう。

Scoreの加算だからScoreModelクラスがやる！という思考回路で書いたのだと思いますが、スコア自体はJudgementResultクラスが持っていた方がよさそうです。

もうちょっと踏み込んで考える

そもそもJudgementResultTypeとそれに対応したscoreは本質的には同じことではないでしょうか。 JudgementResultTypeがPerfectならscoreは15といったことが関数のように決定されることが期待されているのです。

つまり、JudgementResultTypeとscoreは関数的なのです。

1対1対応の物を変換するだけのメソッドは、OCPに違反している可能性が高いです。なぜなら、その部分をそのままクラスの中に定義することが可能だからです。

もっと言うと、lagMsとJudgementResultTypeも関数的ではないでしょうか。lagMsが決まればJudgementResultTypeは一意に定まります。

そんなわけで、このクラスは以下のようになっています。

こう見ると、そもそも必要な情報は、LagMs、もしくはJudgementResultTypeだけなのではないでしょうか...？

（この辺りの話は、DBの主キー等に通ずる部分があると思います。参考までに。）

改善例

実際にクラス内に定義してみましょう。今回はScoreのみを必要な情報としてとらえ、書いてみました。

using UniRx;
using UnityEngine;

namespace Scores
{
    public enum JudgeType
    {
        Perfect,
        Great,
        Good,
        Bad,
        Miss,
    }

    public interface IJudgement
    {
        int Score { get; }
    }


    public class Perfect : IJudgement
    {
        public int Score => 15;
    }

    public class Great : IJudgement
    {
        public int Score => 10;
    }

    public class Good : IJudgement
    {
        public int Score => 5;
    }

    public class Bad : IJudgement
    {
        public int Score => 1;
    }

    public class Miss : IJudgement
    {
        public int Score => 0;
    }
}

namespace Scores
{
    public class ScoreModel : MonoBehaviour
    {
        public IReadOnlyReactiveProperty<int> Score => _score;
        private readonly ReactiveProperty<int> _score = new ReactiveProperty<int>(0);

        // ...


        public void AddScore(IJudgement judgement)
        {
            _score.Value += judgement.Score;
        }
    }
}

interfaceを利用して上手く実装を分離することが出来ました。また、不必要な情報も減らすことができ、より本質的なコードになったと思います。

ここまできたらstructとかrecordとかにした方が良いかも...と思いましたがそこは別の話になるので今回は割愛（まだあんまりstruct理解できてないし）。

いっぱいクラスできるから嫌！と言う方は、Judgementを5回newしてDIする、みたいな使い方もありかと思います（具体的な実装は割愛します）。

数値のハードコーディングが気になる場合は、ScriptableObjectを利用してクラスの生成時にそこから情報を取ってくるといいのかもしれません。

Qemelly(けめる)のプログラム備忘録

Unity / AtCoderについて書きます

【SOLID原則】OCPについてちゃんと考えたい