Return to Revisions

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.

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.