current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 6 down vote favorite

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.

share|improve this question
    
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 –  Malachi 2 days ago
    
@Malachi You're right. I'm sorry. –  WiS3 2 days ago
    
please do post a link in a comment to the new question though –  Malachi 2 days ago
    
Here is the updated version: codereview.stackexchange.com/questions/47737/… –  WiS3 2 days ago
add comment

4 Answers

up vote 7 down vote accepted

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
share|improve this answer
add comment
up vote 5 down vote

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.

share|improve this answer
    
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. –  WiS3 yesterday
add comment
up vote 2 down vote

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

share|improve this answer
    
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) –  WiS3 2 days ago
    
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). –  Nikita Brizhak yesterday
1  
If < and == are used for completely different things, how would you implement <=? Or would you just disallow that by not implementing <=? –  svick yesterday
    
@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 yesterday
    
@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. –  Travis Snoozy yesterday
show 2 more comments
up vote -1 down vote

the only thing that I am seeing at first glance is that you take the rnd variable out of the if statement and put it in a higher scope, then you could use it other places as well.

Have you tested this bit of code?

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

because I am thinking that you will come up with the same value in rng when it is assigned to f and s so in other words it will always end the same way every time you go through the if statement while f==null

int rng = rnd.Next(0,2);

is the same as saying 

int rng = 1;

I think you should test that if statement and make sure that it is doing what you want it to do.

share|improve this answer
    
I have checked and the random part of the code is working. The rng int value must be the same while inside the if statement. –  WiS3 2 days ago
    
The way Random is used here is wrong, but it won't return the same value every time. –  svick 2 days ago
1  
@svick please explain why it is wrong. –  WiS3 2 days ago
    
random returns a value between 0 and 1, and it doesn't return an integer, I think it returns a float. –  Malachi 2 days ago
    
the stuff inside the if statement actually does what it is supposed to do, I was just thinking it was supposed to do the opposite. I don't see why you need to set both f and s if you know who is first, then you automatically know who is second. –  Malachi 2 days ago
show 2 more comments

Your Answer

 
discard

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.