Simplistic (dumb) Card Fighter

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 
    /// Returns a new Assets instance with warriors and mages incremented by the number of kings.
    /// 
    /// 
    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;
priva

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 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 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 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

• The game driving the tournament

• The Tournament driving the pairing of combatants

• The Combatants doing the fighting.

• Better separation of concerns

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

-
[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.

StackExchange Code Review Q#49492, answer score: 5