Simplistic (dumb) Card Fighter

Question

I wanted to play with value types for Simon's challenge; I came up with this quick-and-dirty, basic implementation of a card-fighting game:

I started with a value type to hold assets - I'd reuse that type for player status, card cost and card effects:

public struct Assets
{
    private readonly int _warriors;
    private readonly int _mages;
    private readonly int _kings;
    private readonly int _health;

    public int Warriors { get { return _warriors; } }
    public int Mages { get { return _mages; } }
    public int Kings { get { return _kings; } }
    public int Health { get { return _health; } }

    public Assets(int warriors = 0, int mages = 0, int kings = 0, int health = 0)
    {
        _warriors = warriors < 0 ? 0 : warriors;
        _mages = mages < 0 ? 0 : mages;
        _kings = kings < 0 ? 0 : kings;
        _health = health < 0 ? 0 : health;
    }

    /// <summary>
    /// Returns a new Assets instance with warriors and mages incremented by the number of kings.
    /// </summary>
    /// <returns></returns>
    public Assets GetKingsBenediction()
    {
        return new Assets(warriors: _warriors + _kings,
                          mages: _mages + _kings,
                          kings: _kings,
                          health: _health);
    }

    public override string ToString()
    {
        return string.Format("Health: {0}\nWarriors: {1}\nMages: {2}\nKings: {3}", _health, _warriors, _mages, _kings);
    }
}

CardEffect is also a struct, and might act upon one or more of either the player's or his opponent's assets:

public struct CardEffect
{
    public static readonly int MaxEffect = 255;
    public static readonly Assets NoOp = new Assets();

    private readonly Assets _player;
    private readonly Assets _opponent;

    public Assets PlayerAssets { get { return _player; } }
    public Assets OpponentAssets { get { return _opponent; } }

    public CardEffect(Assets player, Assets opponent)
    {
        _player = player;
        _opponent = opponent;
    }
}

A GameCard is nothing more than a Name, a Cost and an Effect then:

public class GameCard
{
    private readonly string _name;
    private readonly Assets _cost;
    private readonly CardEffect _effect;

    public string Name { get { return _name; } }
    public Assets Cost { get { return _cost; } }
    public CardEffect Effect { get {return _effect;} }

    public GameCard(string name, Assets cost, CardEffect effect)
    {
        _name = name;
        _cost = cost;
        _effect = effect;
    }
}

Here's the Game class, showing the GameCard constructor in action:

public class Game
{
    public static readonly Assets StartAssets = new Assets(warriors: 5, mages: 3, kings: 1, health: 30);

    // todo: make non-static, implement deck & player hand
    public static IEnumerable<GameCard> _cards = new[] 
    {
        new GameCard("Attack", new Assets(warriors: 3), new CardEffect(CardEffect.NoOp, new Assets(health: 5))),
        new GameCard("Recruit", new Assets(mages: 2), new CardEffect(new Assets(warriors: 5), CardEffect.NoOp)),
        new GameCard("Panic Kingdom", new Assets(kings: 1), new CardEffect(new Assets(warriors: 10, mages: 10), CardEffect.NoOp)),
        new GameCard("Pick a King, Any King", new Assets(warriors: 6, mages: 6), new CardEffect(new Assets(kings: 1), CardEffect.NoOp)),
        new GameCard("Heal Me", new Assets(mages: 2), new CardEffect(new Assets(health: 4), CardEffect.NoOp)),
        new GameCard("All-In!", new Assets(warriors: 10), new CardEffect(CardEffect.NoOp, new Assets(health: 15))),
        new GameCard("Mega Heal", new Assets(mages: 5), new CardEffect(new Assets(health: 11), CardEffect.NoOp)),
        new GameCard("Transforming", new Assets(warriors: 2), new CardEffect(new Assets(mages: 2), CardEffect.NoOp)),
        new GameCard("Wizards in the City", new Assets(warriors: 2), new CardEffect(new Assets(mages: 4), CardEffect.NoOp)),
        new GameCard("Slay Kingdom", new Assets(warriors: 7, mages: 7), new CardEffect(CardEffect.NoOp, new Assets(warriors: CardEffect.MaxEffect,mages: CardEffect.MaxEffect)))
    };

    public void Run(IEnumerable<PlayerBase> players)
    {
        while (players.Count(p => !p.IsDead) > 1)
        {
            foreach (var player in players)
            {
                if (!player.IsDead)
                {
                    player.Play(players);
                }

                if (!player.IsDead && players.Count(p => !p.IsDead) == 1)
                {
                    Console.WriteLine("{0} wins!", player.Name);
                    Console.ReadLine();
                    break;
                }
            }
        }
    }
}

---

I've coded lots of logic inside the PlayerBase abstract class, but the hand isn't done at this point - I just made Game._cards public static so I could have a bunch of cards to play with:

public abstract class PlayerBase
{
    private readonly string _name;
    private Assets _assets;

    protected PlayerBase(string name, Assets assets)
    {
        _name = name;
        _assets = assets;
    }

    public string Name { get { return _name; } }
    public Assets Assets { get { return _assets; } set { _assets = value; IsDead = _assets.Health == 0; } }

    public bool IsDead { get; private set; }

    public void Play(IEnumerable<PlayerBase> players)
    {
        // todo: draw a card
        _assets = _assets.GetKingsBenediction();
        Console.WriteLine("{0} assets:", Name);
        Console.WriteLine(_assets);

        // todo: implement hand
        var hand = Game._cards; // public static - todo: make private, non-static

        var card = PickCard(hand);
        if (PayCardCost(card))
        {
            var opponent = PickOpponent(card, players.Where(player => !player.Equals(this)));
            PlayCard(card, opponent);
        }
        else
        {
            Console.WriteLine("Cannot play that card. Skipping turn.");
        }
    }

    /// <summary>
    /// Decide which on-hand card to play.
    /// </summary>
    /// <returns></returns>
    protected abstract GameCard PickCard(IEnumerable<GameCard> hand);

    protected abstract PlayerBase PickOpponent(GameCard card, IEnumerable<PlayerBase> opponents);

    private bool PayCardCost(GameCard card)
    {
        if (_assets.Warriors - card.Cost.Warriors >= 0
         && _assets.Mages - card.Cost.Mages >= 0
         && _assets.Kings - card.Cost.Kings >= 0
         && _assets.Health - card.Cost.Health >= 0)
        {
            _assets = new Assets(warriors: _assets.Warriors - card.Cost.Warriors,
                                 mages: _assets.Mages - card.Cost.Mages,
                                 kings: _assets.Kings - card.Cost.Kings,
                                 health: _assets.Health - card.Cost.Health);
            return true;
        }
        else
        {
            return false;
        }
    }

    private void PlayCard(GameCard card, PlayerBase opponent)
    {
        if (!card.Effect.PlayerAssets.Equals(CardEffect.NoOp))
        {
            _assets = new Assets(
                        warriors: _assets.Warriors + card.Effect.PlayerAssets.Warriors,
                        mages: _assets.Mages + card.Effect.PlayerAssets.Mages,
                        kings: _assets.Kings + card.Effect.PlayerAssets.Kings,
                        health: _assets.Health + card.Effect.PlayerAssets.Health);
        }

        if (!card.Effect.OpponentAssets.Equals(CardEffect.NoOp))
        {
            opponent.Assets = new Assets(
                            warriors: opponent.Assets.Warriors - card.Effect.OpponentAssets.Warriors,
                            mages: opponent.Assets.Mages - card.Effect.OpponentAssets.Mages,
                            kings: opponent.Assets.Kings - card.Effect.OpponentAssets.Kings,
                            health: opponent.Assets.Health - card.Effect.OpponentAssets.Health);
        }
    }

---

The ConsolePlayer being used merely picks the first card in the "hand", and always picks the first opponent - as a result the two players fight each others to death using nothing other than "Attack" cards, but that's enough to say that, except for the "hand", everything works:

Output:

Player1 assets:
Health: 30
Warriors: 6
Mages: 4
Kings: 1
Player1 plays Attack.
Player2 assets:
Health: 25
Warriors: 6
Mages: 4
Kings: 1
Player2 plays Attack.
Player1 assets:
Health: 25
Warriors: 4
Mages: 5
Kings: 1
Player1 plays Attack.
Player2 assets:
Health: 20
Warriors: 4
Mages: 5
Kings: 1
Player2 plays Attack.
Player1 assets:
Health: 20
Warriors: 2
Mages: 6
Kings: 1
Player1 plays Attack.
Cannot play that card. Skipping turn.
Player2 assets:
Health: 20
Warriors: 2
Mages: 6
Kings: 1
Player2 plays Attack.
Cannot play that card. Skipping turn.
Player1 assets:
Health: 20
Warriors: 3
Mages: 7
Kings: 1
Player1 plays Attack.
Player2 assets:
Health: 15
Warriors: 3
Mages: 7
Kings: 1
Player2 plays Attack.
Player1 assets:
Health: 15
Warriors: 1
Mages: 8
Kings: 1
Player1 plays Attack.
Cannot play that card. Skipping turn.
Player2 assets:
Health: 15
Warriors: 1
Mages: 8
Kings: 1
Player2 plays Attack.
Cannot play that card. Skipping turn.
Player1 assets:
Health: 15
Warriors: 2
Mages: 9
Kings: 1
Player1 plays Attack.
Cannot play that card. Skipping turn.
Player2 assets:
Health: 15
Warriors: 2
Mages: 9
Kings: 1
Player2 plays Attack.
Cannot play that card. Skipping turn.
Player1 assets:
Health: 15
Warriors: 3
Mages: 10
Kings: 1
Player1 plays Attack.
Player2 assets:
Health: 10
Warriors: 3
Mages: 10
Kings: 1
Player2 plays Attack.
Player1 assets:
Health: 10
Warriors: 1
Mages: 11
Kings: 1
Player1 plays Attack.
Cannot play that card. Skipping turn.
Player2 assets:
Health: 10
Warriors: 1
Mages: 11
Kings: 1
Player2 plays Attack.
Cannot play that card. Skipping turn.
Player1 assets:
Health: 10
Warriors: 2
Mages: 12
Kings: 1
Player1 plays Attack.
Cannot play that card. Skipping turn.
Player2 assets:
Health: 10
Warriors: 2
Mages: 12
Kings: 1
Player2 plays Attack.
Cannot play that card. Skipping turn.
Player1 assets:
Health: 10
Warriors: 3
Mages: 13
Kings: 1
Player1 plays Attack.
Player2 assets:
Health: 5
Warriors: 3
Mages: 13
Kings: 1
Player2 plays Attack.
Player1 assets:
Health: 5
Warriors: 1
Mages: 14
Kings: 1
Player1 plays Attack.
Cannot play that card. Skipping turn.
Player2 assets:
Health: 5
Warriors: 1
Mages: 14
Kings: 1
Player2 plays Attack.
Cannot play that card. Skipping turn.
Player1 assets:
Health: 5
Warriors: 2
Mages: 15
Kings: 1
Player1 plays Attack.
Cannot play that card. Skipping turn.
Player2 assets:
Health: 5
Warriors: 2
Mages: 15
Kings: 1
Player2 plays Attack.
Cannot play that card. Skipping turn.
Player1 assets:
Health: 5
Warriors: 3
Mages: 16
Kings: 1
Player1 plays Attack.
Player1 wins!

public static stuff that works around the absence of a deck and of a hand aside, is everything where it belongs? I don't code structs very often, guts tell me I've stuffed too much logic in Assets... I don't like that I have tied PlayerBase to the Console, would it be cleaner to have Game listen for player events and write to console accordingly?

Answer 1

this:

 if (_assets.Warriors - card.Cost.Warriors >= 0
     && _assets.Mages - card.Cost.Mages >= 0
     && _assets.Kings - card.Cost.Kings >= 0
     && _assets.Health - card.Cost.Health >= 0)

Has no business in the PlayerBase class. In as much as GameCard contains both of these objects, this logic should be there as well. And of course it needs to be encapsulated w/ descriptive name.

Edit

Explore the idea of having Asset implement IComparable, then the above is the meat of CompareTo(). Then GameCard can also implement IComparable and can call Asset.CompareTo it it's own implementation. Easy Peasy.

End Edit

Fractal OO

Go deep w/ abstraction/encapsulation. For example:

 public void Run(IEnumerable<PlayerBase> players)
{
    while (players.Count(p => !p.IsDead) > 1)
    {
       //redacted
            if (!player.IsDead && players.Count(p => !p.IsDead) == 1)
            { // redacted
            }
        }
    }
}

could read more abstractly:

public void Run(IEnumerable<PlayerBase> players)
{
    while (CombatStillPossible)
    {
       //redacted
            if (LastPlayerStanding)
            { // redacted
            }
        }
    }
}

Run() is fuzzy

Looking at the original code as just the method parameters together w/ the overall control logic, it is not at all clear what I'm seeing. Also, the seeming nested-repeated logic is making me wonder about flawed logic or poorly structured logic.

I wonder if you get the outlier conditions out of the way first thing then all the logic is clearer.

 public void Run(IEnumerable<PlayerBase> players)
{
    if(NoLivingPlayers) { etc, etc, return;}  // covers a null argument too
    if(OnePlayerAlive) { // we have a winner, return;} // covers the case of a collection of 1 player

    while (CombatStillPossible)
    {
        // internal logic would have avoided the "3 players bug"
        // internal logic keeps paring players and fighting each pair
        // until only one is left.

        Tournament meatGrinder = new Tournament(); // pass in all the players
        PlayerBase winner = meatGrinder.Fight();

        // a null winner means everyone is dead
        // "winner" could be class variable so it can be evaluated in
        // NoLivingPlayers, etc.

        if(NoLivingPlayers) {// oh well, etc.; continue;}
        if (LastPlayerStanding)
        { // winner, etc.;}
    }
}

Sure, the above is putting off actually doing anything, but we're going fractal here. Tournament will internally pair players...

public class Combatants {
    public PlayerBase Player1 { get; protected set; }
    public PlayerBase Player2 { get; protected set; }

    public Combatants(PlayerBase player1, PlayerBase player2) {

    }

    public PlayerBase Fight() {} // here is the logic of single combat.
}

What We Have

1. The game driving the tournament
2. The Tournament driving the pairing of combatants
3. The Combatants doing the fighting.
4. Better separation of concerns

5. We coded at appropriate levels of abstraction w/in each class. A.K.A. we pushed details down.

6. [edit] One more thing.... the while may now seen superfluous, but it got me to thinking that Tournament can now be passed as a parameter and now we can have game variations. Holy Inversion of Control, Batman.

Answer 2

Not going to review this code just yet, but I've found a funny bug, by adding a 3rd player:

static void Main(string[] args)
{
    var game = new Game();
    var players = new[] 
            { 
                new ConsolePlayer("Player1", Game.StartAssets), 
                new ConsolePlayer("Player2", Game.StartAssets),
                new ConsolePlayer("Player3", Game.StartAssets)
            };

    game.Run(players);
}

This would run an infinite loop, and eventually hit an overflow exception when players have more mages and warriors than C# can count.

The bug is on this line:

var opponent = PickOpponent(card, players.Where(player => !player.Equals(this)));

PickOpponent gets to fight a dead player!

This fixes is:

var opponent = PickOpponent(card, players.Where(player => !player.IsDead && !player.Equals(this)));

---

I don't like that PickOpponent is provided with "opponents" - it should take "players", and determine itself what players are possible opponents.