Initial starting player - v2

This is the updated code using all suggestions from the previous question that I made here.

I have revised the naming of the variables, and i have included the classes that this part of code is using, in order to have a better understanding.

I'm making a game battle system, where there are 2 players.

It is like a round based game.

• Every player can have multiple units to control, but can only use one of them each round..
• Every unit has an int CurrentSpeed variable;

I need to have in 2 variables who attacks first (Player firstPlayer) and who attacks second (Player secondPlayer), based on the speed of the active Unit.

At the moment I have this code:

public class Player {
public int Id { get; set; }
public string Name { get; set; }
public List<Unit> Party { get; set; }
public Unit Active { get; set; }
}

public class Unit {
public string Name;
public int InitialSpeed { get; set; }
public int CurrentSpeed { get; set; }
}

public class BattleSystem {

private void BattleTurn(){
Player firstPlayer = null; //Player attacking first
Player secondPlayer = null; //Player attacking second

//Check the speed of the units, and determine who attack first
firstPlayer = player_1.Active.CurrentSpeed > player_2.Active.CurrentSpeed ? player_1 : player_1.Active.CurrentSpeed < player_2.Active.CurrentSpeed ? player_2 : null;

//if firstPlayer is null (active units have the same CurrentSpeed)
if(firstPlayer == null){
//Randomize who goes first
System.Random rnd = new System.Random();
int rng = rnd.Next(2);
firstPlayer = rng == 0 ? player_1 : player_2;
secondPlayer = rng == 0 ? player_2 : player_1;
}else{
secondPlayer = firstPlayer.Id == player_1.Id ? player_2 : player_1;
}
}
}


The part of determine the firstPlayer is ok, but I don't like the checks and assignments of the secondPlayer, like checking the ids.

I need tips on how I can code this in a better way.

-

Do without the ternary expressions: because a ternary expression is very good for initializing one variable, but is much less good for initializing two variables.

// Declared but not initialized (to null), in order to guarantee that
// they are each initialized in the subsequent if statements.
Player firstPlayer;
Player secondPlayer;

if (player_1.Active.CurrentSpeed > player_2.Active.CurrentSpeed)
{
firstPlayer = player_1;
secondPlayer = player_2;
}
else if (player_1.Active.CurrentSpeed < player_2.Active.CurrentSpeed)
{
firstPlayer = player_2;
secondPlayer = player_1;
}
else
{
//Randomize who goes first
... etc ...
}

-

• There is little point in recreating Random variable every time you need it. You should consider saving it to the private field.
• You should also probably switch to C# coding style from what seems to be a Java style when it comes to using braces. Check ChrisW's example - opening braces go to the new line. Same goes for operators such as else.
• As a rule of a thumb - you should not expose public fields (unless you have a reason to). So Unit.Name should probably be an auto-property as well.
• It is a good idea to always have Id set accesor private.
“There is little point in recreating Random” More than that: it's wrong, as I explain in my answer to the previous question. –  svick Apr 21 '14 at 11:34