Initial starting player

Question

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

• Every player can have multiple units to control, but only one at a time.
• Every unit has an int curSpeed variable;

I need to have in 2 variables who attacks first (Player f) and who attacks second (Player s), based on the speed of the current controlling unit.

At the moment I have this code:

• player_1 and player_2 are instances of Player Class
• current, is a instance of Unit Class, it is the current controlling unit.

Player f = null; //Player attacking first
Player s = null; //Player attacking second
//Check the speed of the units, and determine who attack first
f = player_1.current.curSpeed > player_2.current.curSpeed ? player_1 : player_1.current.curSpeed < player_2.current.curSpeed ? player_2 : null;
//if f is null (units have the same speed)
if(f==null){
 //Randomize who goes first
 System.Random rnd = new System.Random();
 int rng = rnd.Next(0,2);
 f = rng == 0 ? player_1 : player_2;
 s = rng == 0 ? player_2 : player_1;
}else{
 s = f.id == player_1.id ? player_2 : player_1;
}

It is working, but I feel that this is confusing and the 'wrong way' to do it.

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

UPDATE: New version of the code including all the suggestions can be found here.

• Malachi: if you want to show the new code, please post another question. you would invalidate the answers if you edit in the changes to this question
• Fr0z3n: @Malachi You're right. I'm sorry.
• Malachi: please do post a link in a comment to the new question though
• Fr0z3n: Here is the updated version: codereview.stackexchange.com/questions/47737/…

Answer 1

Your code is indeed confusing, primarily because of the naming:

f should be firstPlayer, s should be secondPlayer and curSpeed should be speed or currentSpeed (it depends on whether there are other "speeds" to distinguish from).

Another remark: properties are UpperCamelCase so this

player_1.current.curSpeed

should be

player_1.Current.Speed

Answer 2

The way you're using Random is wrong. Instead of creating a new instance of Random every time you need a new random number, you should create a single instance of Random, store it, and then repeatedly call Next() on that single instance.

The reason for this is that when you create Random using the default constructor, it uses the current time as its seed. But the time it uses has relatively low resolution, so when you create two instances of Random too close to each other, you're going to get the same result.

To make this more concrete, try the following code:

while (true)
{
 var random = new Random();
 Console.Write(random.Next(0, 2));
 Console.Write(" ");
 Thread.Sleep(1);
}

For me, the output is something like:

1 1 1 1 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0

As you can see, this doesn't look very random, with long runs of the same number. (If you remove the sleep, each run is even longer.)

Compare it with this code:

var random = new Random();
while (true)
{
 Console.Write(random.Next(0, 2));
 Console.Write(" ");
 Thread.Sleep(1);
}

Its output:

0 0 0 0 0 1 1 0 0 0 0 0 1 1 1 1 1 1 1 1 0 1 1 0 0 1 1 1 1 0 1 1 1 1 0 1 0 1 0 0 1 1 1 1 0 1 0 0 1 1 1 1 0 1 0 1 1 1 1 0 1 1 1 1 0 0 1 0 1 0 0 1 1 0 1 0 1 1 1 1 1 1 1 1 1 0 0 1 1 1 1 0 0 1 0 1 1 1 1 0 0 1 1 0 0 1 0 0 0 1 1 0 0 0 1 0 0 0 0 1 1 1 1 1 0 1 1 1 1 1 0 1 1 0 1 1 0 1 0 1 0 0 1 1 0 1 0 1 0 0 1 1 1 1 1 0 1 1 1 1 0 1 0 0 0 1 0 1 1 1 0 0 1 1 1 0 0 1 1 1 1 0 0 1 0 0 0 0 1 0 1 0 1 1 0 1 0 0 0 0

Now this looks much more random. And this time, removing the sleep won't change the result.

• Fr0z3n: I know that, anyway it is not my case, because this function it is not inside a loop. It is not called very often in time so i wont get the same result. Anyway i have moved the random instance outside the method, just to be sure.

Answer 3

I really like this question for its simplicity and depth; getting two variables into a hi/lo state is something you run into all the time. Let's start with the original assignment below...

f = player_1.current.curSpeed > player_2.current.curSpeed ? player_1 : player_1.current.curSpeed < player_2.current.curSpeed ? player_2 : null;

It would be nice if you could actually order the players directly, using overloaded operators, so that this would at least reduce to...

f = player_1 > player_2 ? player_1 : player_2;

with the following "if" statement being changed to...

if(player_1.current.curSpeed == player_2.current.curSpeed)

I'm not sure that the value of rng is going to be what you think it is. You probably simply want to use...

rng = rnd.Next(2);

if you're aiming for a value of either 0 or 1 with equal probability. I'd also suggest moving the initial assignment of "f" down into the else block, since now you don't need to check if "f" is null before doing the random-assign.

You'll want to overload ">" and "<" to compare two player's speed (to rank them correctly), and overload "==" to compare two player's IDs. This suggestion may offend some; it is reasonable to insist that if a < b is false, and b < a is false, a == b, and my suggested implementations won't follow that rule. If that is important to you, you could instead write, e.g., a "faster" function for your player class that will return a bool -- a.faster(b) == true would mean A is faster than B. In any case, this would be the modified code using operator overloading.

Player f = null; //Player attacking first
Player s = null; //Player attacking second
//Check the speed of the units, and determine who attack first
//if units have the same speed...
if(player_1.current.curSpeed == player_2.current.curSpeed){
 //Randomize who goes first
 System.Random rnd = new System.Random();
 int rng = rnd.Next(0,2);
 f = rng == 0 ? player_1 : player_2;
 s = rng == 0 ? player_2 : player_1;
}else{
 f = player_1 > player_2 ? player_1 : player_2;
 s = f == player_1 ? player_2 : player_1;
}

As an afterthought, the "current.curSpeed" seems redundant (the current current speed?), and very wordy. I'd suggest either dropping the "current" altogether (really think hard about if it's necessary), OR change the "curSpeed" to just "Speed."

• Fr0z3n: I know, i need to revise the naming. Anyway current is needed, because it is actually the active Unit class instance that the player is controlling, in other words: the players have 3 monsters, but only one of them can be used each round. So i need that variable to calculate wich one attack first, based on their curSpeed. (i do have initialSpeed and curSpeed inside my Unit class to track the initial value and the current value, since it can be changed during the battle)
• Nikita B: I don't like this solution. Equality operators should make sense for objects they are applied to. They do make sense for int value (speed), but they dont for Players. There is no way to tell on what basis is one payer being compared to another (is it id, or score, or hit points, or level, or - would be my last guess - current unit's speed), without digging into implementation details, which is a sign of bad design. Implementing an IComparer instead is a better soultion in my opinion (if this kind of comparison is used often enough).
• svick: If < and == are used for completely different things, how would you implement <=? Or would you just disallow that by not implementing <=?
• Travis Snoozy: @NikitaBrizhak First, the equality is clear: two players are equal if and only if their IDs are equal. WRT comparison criteria, I consider that to be encapsulation. If A < B, A goes first. If the rules determining how or why A < B is outside the scope of this code -- that is to say, it's the player's responsibility to determine rank -- then this approach is correct. Implementing IComparer is exactly as wrong (or right) as overloading "<", ">", and "==" in that case.
• Travis Snoozy: @svick You cannot sanely implement <= or >= in these cases, and thus should not implement those operators. My answer addresses this obliquely ("[...]it is reasonable to insist that if a < b is false, and b < a is false, a == b[...]"). To reiterate, if this is important to you, you should use a (non-operator) function name off the class, like "bool actsBefore(Player p)". The reason I prefer "<" in this particular code is that the hi-lo code looks a lot more like the reusable generic case when normal comparison operators are used.