Unity（C#）可変長引数＆型の変わる関数を作りたい。

前提・実現したいこと

Unity（C#）で、可変長引数＆型の変わる関数を作成したいと思っております。
用途は、カードゲームの能力の実行に使いたいと思っています。
各能力で必要な引数が異なるため、どう実現するのが一番スマートなのかアドバイスをいただければ、と思います。
このまま能力を増やしていくとどんどん引数が多くなってきて、メンテナンスがしづらい関数になりそうなのです。

該当のソースコード

C#
1public IEnumerator RunAbility(string[] ability, string[] param, CardController card, UnitController unit, LandController land,bool isPlayer){
2    // アビリティを実行する
3    int i = 0;
4    foreach(string abl in ability){
5        // カードドロー
6        if( abl == "draw" ){
7            yield return StartCoroutine(DrawHand(int.Parse(param[i]),isPlayer));
8        }
9        // ユニットへのダメージ
10        else if( abl == "damage" ){
11            yield return StartCoroutine(GameManager.instance.UnitSkillDamage(unit,int.Parse(param[i])));
12        }
13        // 先制
14        else if( abl == "at_preemptive" ){
15            unit.SetCanAttack(true);
16        }
17        // 土地レベルアップ
18        else if( abl == "summon_land_up" ){
19            if(land!=null){
20                land.LvUP(1);
21            }
22        }
23        i++;
24    }
25}

試したこと

◆可変長引数

C#
1public IEnumerator RunAbility(string[] ability, string[] param, string[] args)

←これは型変換ができないためどうしようもないように思えました。

◆ジェネリック
自分でジェネリックを組んだことがなく、初心者マークをいれるべきかどうか迷いましたが、
ジェネリックだと下記のようなクラスのメソッドをどのように呼び出せばいいのかわかりませんでした。

C#
1land.LvUP(1);

◆関数のオーバーロード
これはできそうだぞ、と思ったのですが、
今後引数がどんどん増えてきたときに最善手なのかどうかわからないでいます。

BluOxy

2020/08/08 20:29

クラス設計や命名を意識すると分岐や引数がそのように増えることはないと思います。

moon_peta

2020/08/09 04:13

ありがとうございます！返信を見て、そもそもの設計がまずいことに気づきました。・直前の効果の対象（引数にいれていた「〇〇Controller」）はバトルのシチュエーションを管理しているクラスに保存する・個々の能力の実行は関数で分ける・条件文はFuncなどに置き換える（←今ここ調べてます）といった感じで組み替えていて、すっきりしそうな見通しが立ってきました。

行動規範の内容に同意します

回答2件

自己解決

無事解決しました。下記のように設計しなおしました。

C#
1Dictionary<string,System.Func<string,bool,IEnumerator>> func = new Dictionary<string,System.Func<string,bool,IEnumerator>>();
2
3// 能力と関数を紐づけてDictionaryに登録
4void Start()
5{
6    func.Add("draw",            AbilityDraw);
7    func.Add("damage",          AbilityDamage);
8    func.Add("at_preemptive",   AbilityAtPreemptive);
9    func.Add("summon_land_up",  AbilitySummonLandUp);
10}
11
12// アビリティの実行
13public IEnumerator RunAbility(string[] ability, string[] param, CardController card, bool isPlayer){
14    // アビリティを実行する
15    int i = 0;
16    foreach(string abl in ability){
17        if(abl != null){
18            yield return func[abl](param[i],isPlayer);
19        }
20        i++;
21    }
22    // カード削除とMP消費の処理は省略
23}
24
25// カードドロー
26IEnumerator AbilityDraw(string param,bool isPlayer)
27{
28    yield return StartCoroutine(DrawHand(int.Parse(param),isPlayer));
29}
30
31// ユニットへのダメージ
32IEnumerator AbilityDamage(string param,bool isPlayer)
33{
34    UnitController unit = GameManager.instance.BattleSituation.UnitTarget;
35    yield return StartCoroutine(GameManager.instance.UnitSkillDamage(unit,int.Parse(param)));
36}
37
38// 先制
39IEnumerator AbilityAtPreemptive(string param,bool isPlayer)
40{
41    UnitController unit = GameManager.instance.BattleSituation.UnitTarget;
42    unit.SetCanAttack(true);
43    yield return null;
44}
45
46// 土地レベルアップ
47IEnumerator AbilitySummonLandUp(string param,bool isPlayer)
48{
49    LandController land = GameManager.instance.BattleSituation.LandTarget;
50    if(land!=null){
51        land.LvUP(1);
52    }
53    yield return null;
54}

foreachのループもう少し簡略化できそうな気がしますし、
引数（isPlayer）もなくせるんじゃないかと思いましたが、
今回の目的（能力の設計）は達成できたので、これで解決とします。

どうもありがとうございました！

投稿2020/08/09 05:37

moon_peta

総合スコア1

BluOxy

2020/08/09 10:57 編集

自己解決済みではありますが、以下に改善点を挙げました。思うところがあれば、設計を見直すための材料にしてください。 1. クラス単位で分割できると良いかもしれません。今回の場合、修正のたびそのクラス内のメソッドを追加する必要があるように見えるので、そのようにして修正を加えていくとやがてコード量と役割が肥大化する未来が見えます 2. SOLID原則で調べると幸せになれるかもしれません。クラス単位での分割もその原則に則るもので、SOLID原則の5つの原則の内、開放閉鎖の原則のことに関するコメントです。他にもSOLID原則に則っていないもので検討できる部分があればそのようにすると良いと思います 3. 命名が気になりました。 paramにはどのような値が入るのかが気になります。paramの目的によってはabilityとparamを1つのオブジェクトにまとめて送った方が分かりやすい気がします 4. Abilityに属するものが多すぎるように見えます。これらは全て能力なのでしょうか。あくまでDrawは「ユーザーの行動」で、DamageやAtPreemptiveは「ユニットの行動」に見えます。SummonLandUpは何に属するかわかりませんが、Abilityに属するのかが疑問でした。DrawやDamage、AtPreemptive、SummonLandUpを全てAbilityとしてまとめることでかえって複雑になっていないでしょうか

moon_peta

2020/08/09 12:46

コメントありがとうございます！＞1. クラス単位で分割ご指摘の通り、この作りだとクラス内メソッドがどんどん増えていきますね…… ＞2. SOLID原則こちら存じませんでした。調べたところ、クラスの見直しから必要そうですね。開放閉鎖の原則で調べていたら、昔一度勉強した記憶のある Strategyパターンが出てきたので、もう一度勉強しなおして組んでみます。＞3. 命名が気になりました。 abilityはカードを引く、ダメージを与えるなどの能力。 paramはカードを引くときの枚数、ダメージを与えるときのダメージ数などで使用しています。対の関係なのでおっしゃるとおり一つにまとめたほうが良いですね。＞4. Abilityに属するものが多すぎるように見えます。「ユーザーの行動」と「ユニットの行動」についておっしゃるとおりです。いただいたコメントを踏まえ、改善に挑戦してみて、そこで、もしまた躓いたら、新規で質問させていただこうと思います。どうもありがとうございました。

行動規範の内容に同意します

BluOxyさんの返信をみてそもそもの設計がまずいことに気づき、引数や型が異ならないようにプログラムを書き換えました。

・直前の効果の対象（引数にいれていた「〇〇Controller」）はバトルのシチュエーションを管理しているクラスに保存する
・個々の能力の実行は関数で分ける
・条件文はFuncなどに置き換える（←今ここ調べてます）

C#
1public IEnumerator RunAbility(string[] ability, string[] param, CardController card, bool isPlayer){
2
3    // ここの部分をFuncで書き換えて条件文をなくせるか検討中
4    int i = 0;
5    foreach(string abl in ability){
6        if( abl == "draw" ){
7            yield return AbilityDraw(param[i],isPlayer);
8        }
9        else if( abl == "damage" ){
10            yield return AbilityDamage(param[i],isPlayer);
11        }
12        // 先制
13        else if( abl == "at_preemptive" ){
14            yield return AbilityAtPreemptive(param[i],isPlayer);
15        }
16        else if( abl == "summon_land_up" ){
17            yield return AbilitySummonLandUp(param[i],isPlayer);
18        }
19        i++;
20    }
21}

C#
1IEnumerator AbilityDraw(string param,bool isPlayer)
2{
3    yield return StartCoroutine(DrawHand(int.Parse(param),isPlayer));
4}

C#
1IEnumerator AbilityDamage(string param,bool isPlayer)
2{
3    UnitController unit = GameManager.instance.BattleSituation.UnitTarget;
4    yield return StartCoroutine(GameManager.instance.UnitSkillDamage(unit,int.Parse(param)));
5}