Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

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

share|improve this question
up vote 6 down vote accepted

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;
    //Randomize who goes first
    ... etc ...
share|improve this answer

In addition to ChrisW's answer:

  • 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.
share|improve this answer
“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
For the brackets, for me it is really a personal coding style. I always coded in that way, so for me it is more readable. Can you please explain better you 3rd point? I do set public fields because i do have to set the values when i initialize a new instance of the class. If i set them private i cannot access them outside of that class. (I'm still learning, so I'm probably doing it wrong :P) Thanks. – Fr0z3n Apr 21 '14 at 12:16
@WiS3 If you use a Microsoft IDE for editing, the braces snap to separate lines as you edit the file (unless you override the default formatting options). – ChrisW Apr 21 '14 at 13:40
@svick I do use MonoDevelop mainly (because it integrate very well with Unity Engine). – Fr0z3n Apr 21 '14 at 13:44
@WiS3, I do understand that. Nonetheless, you will eventually work in a team, and you will have to play by the rules. So it is good idea to keep your mind flexible right know and learn the ability to change your coding style depending on language. The longer you keep this "i like it that way"-mindset, the harder it will be to drop this habit later on. :) – Nikita Brizhak Apr 22 '14 at 6:22

Your Answer


By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.