Is there a way to make this more efficient?

Question

So I am working on a text adventure to improve my programming skills (Just a beginner) and I was working on a new combat system for it because the old one was really boring. So I came across a rock paper scissors system but I wanted something that used a rock paper scissors like system with 5 options for the player to choose from, and the enemy or monster attacking the player.

I used a lot of if statements, which actually didn't take too long, but I'm wondering if there's a better way to do this so that my code is more efficient, and not so large.

        public static void ResultsOfMoves(string PlayerMove, string MonsterMove, Monster CurrentMonster, Weapon CurrentWeapon, Armor CurrentArmor, Player CurrentPlayer)
    {
        //Monster Responses to Player
        if (PlayerMove == "dodge" && MonsterMove == "heavy"||MonsterMove == "stealth")
        {
            if (MonsterMove == "heavy") { MonsterHeavyAttack(); }
            if (MonsterMove == "stealth") { MonsterStealthAttack(); }
        }
        else if (PlayerMove == "charge" && MonsterMove == "dodge"||MonsterMove == "stealth")
        {
            if (MonsterMove == "dodge") { MonsterDodge(); }
            if (MonsterMove == "stealth") { MonsterStealthAttack(); }
        }
        else if (PlayerMove == "block" && MonsterMove == "charge" || MonsterMove == "dodge")
        {
            if (MonsterMove == "charge") { MonsterChargeAttack(); }
            if (MonsterMove == "dodge") { MonsterDodge(); }
        }
        else if (PlayerMove == "heavy" && MonsterMove == "block" || MonsterMove == "charge")
        {
            if (MonsterMove == "block") { MonsterBlock(); }
            if (MonsterMove == "charge") { MonsterChargeAttack(); }
        }
        else if (PlayerMove == "stealth" && MonsterMove == "heavy" || MonsterMove == "block")
        {
            if (MonsterMove == "heavy") { MonsterHeavyAttack(); }
            if (MonsterMove == "block") { MonsterBlock(); }
        }

        //Players Responses To Monster
        if (MonsterMove == "dodge" && PlayerMove == "heavy" || PlayerMove == "stealth")
        {
            if (PlayerMove == "heavy") { MonsterHeavyAttack(); }
            if (PlayerMove == "stealth") { MonsterStealthAttack(); }
        }
        else if (MonsterMove == "charge" && PlayerMove == "dodge" || PlayerMove == "stealth")
        {
            if (PlayerMove == "dodge") { MonsterDodge(); }
            if (PlayerMove == "stealth") { MonsterStealthAttack(); }
        }
        else if (MonsterMove == "block" && PlayerMove == "charge" || PlayerMove == "dodge")
        {
            if (PlayerMove == "charge") { MonsterChargeAttack(); }
            if (PlayerMove == "dodge") { MonsterDodge(); }
        }
        else if (MonsterMove == "heavy" && PlayerMove == "block" || PlayerMove == "charge")
        {
            if (PlayerMove == "block") { MonsterBlock(); }
            if (PlayerMove == "charge") { MonsterChargeAttack(); }
        }
        else if (MonsterMove == "stealth" && PlayerMove == "heavy" || PlayerMove == "block")
        {
            if (PlayerMove == "heavy") { MonsterHeavyAttack(); }
            if (PlayerMove == "block") { MonsterBlock(); }
        }

    }

There are many ways to make this more efficient. For example if you used enums instead of strings you would be far more compact. Also you could create tables of function pointers (delegates in C#) and just look up the next action in a dictionary and execute it in one line of code. I will let someone more current in C# illustrate that with code. — Mike Wise, May 31 '15 at 04:39
@Makoto: It might be C++, whose `std::string class` supports comparisons using `==`. OP could you please add a language tag? — Jim Lewis, May 31 '15 at 04:42
You should bring this up at codereview.stackexchange.com. One idea would be to use a dictionary that maps pairs of monster and player moves (defined as enums ideally, as suggested by @Mark), to `Response`s, where `Response` is just a parameterless void delegate. Then you'd accept such a pair of a player and a monster move in your method, let's call this struct `Turn`, and you'd just replace your entire method with the following two calls `playerResponses[turn](); monsterResponses[turn]();`. — Asad Saeeduddin, May 31 '15 at 05:06
If the code works as intended and you are looking for improvements, you're welcome to post to Code Review. I would _strongly_ advise to change your title to describe what the code does, if you decide to do that. — Phrancis, May 31 '15 at 05:08
@Makoto In C# strings can be compared with ==, no need for Java-esque `Equals()` — Sami Kuhmonen, May 31 '15 at 05:22
this topic has been thoroughly covered at Programmers, see eg [How to tackle a 'branched' arrow head anti-pattern?](http://programmers.stackexchange.com/q/205803/31260) and _multiple_ questions [linked to it](http://programmers.stackexchange.com/questions/linked/205803?lq=1) — gnat, May 31 '15 at 09:01

score 1 · Accepted Answer · answered May 31 '15 at 06:17

First create a Move enum, rather than using strings:

public enum Moves
{
    Charge,
    Dodge,
    Heavy,
    Steath,
    Block
}

Next, use a Dictionary to determine the moves:

var moveResolution = new Dictionary<Tuple<Moves, Moves>, Action>
{
    { new Tuple<Moves, Moves>(Moves.Dodge, Moves.Heavy), MonsterHeavyAttack },
    { new Tuple<Moves, Moves>(Moves.Dodge, Moves.Steath), MonsterStealthAttack },
    { new Tuple<Moves, Moves>(Moves.Charge, Moves.Dodge), MonsterDodge },
    ...
};

Then to determine the appropriate move, simply do:

var moveCombination = new Tuple<Moves, Moves>(playerMove, monsterMove);
if (moveResolution.ContainsKey(moveCombination))
{
    moveResolution[moveCombination]();
}

This code could then be further improved by replacing the lazy Tuple<Moves, Moves> with a MoveCombination struct. Note, use a struct to ensure the moveResolution.ContainsKey(moveCombination) part works as you need to compare by value, not by reference.

Is there a way to make this more efficient?

1 Answers1