Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

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

I wrote this a while ago but wanted to see if I went about it the right way and if my brain is working correctly. I was thinking about these two projects the other day and I recently released the source. This game was originally for a Windows Form Game Jam so you have full context.

This code worked but I always felt there were inaccuracies in how it handled it. For example, there had been times a rock would clip through a ship.

private void CollisionDetection()
{
    foreach(PictureBox s in rocks)
    {
        //These tell the difference between, I'm not sure if this is all correct, its just what I came up with.
        int Xdifference = (s.Location.X + (s.Width / 2)) - (player.Location.X + (player.Width / 2));
        int Ydifference = (s.Location.Y + (s.Height/ 2)) - (player.Location.Y + (player.Height/ 2));

        //get the actual distance apart
        double dist = Math.Sqrt((Xdifference * Xdifference) + (Ydifference * Ydifference));

        //And finally, does the real comparison
        if(dist < ((s.Width / 2) + (player.Width / 2)) && damagable)
        {
            health -= 10;
            damagable = false;
            impactHit.Play();
        }
    }
}

Full file is found here and full source, here.

share|improve this question
up vote 12 down vote accepted

Collision detection

You can calculate the collision much easier with the Rectangle structure.

Use the Contains method if you are interested in single points that

Determines if the specified point is contained within this Rectangle structure.

var pictureRect = new Rectangle(
    pictureBox.Location.X, 
    pictureBox.Location.Y, 
    pictureBox.Width, 
    pictureBox.Height
);
var isCollision = pictureRect.Contains(player.Location.X, player.Location.Y);

If you want to know whether the rectangles have a collision then use the IntersectsWith method that

Determines if this rectangle intersects with rect.

var isCollision = pictureRect.IntersectsWith(player.Rectangle());

Create and an extension to easier get the rectangle from a PictureBox.

public static Rectangle Rectangle(this PictureBox pictureBox)
{
    return new Rectangle(
        pictureBox.Location.X, 
        pictureBox.Location.Y, 
        pictureBox.Width, 
        pictureBox.Height
    );
}

Naming

foreach(PictureBox s in rocks)

You should use more meaningful names than just s. A variable like pictureBox or pb would be much better because it's derived from the collection. The s does not have any meaning. Although in this case a rock would be perfect.


Health & Damage

health -= 10;

This operation is not clear at first and maybe you have other places where you need to decrease health. Consider using constants and a helper method for this.

private static class Damage
{
    public const int WhenHitRock = 10;
}

void DecreaseHealth(int value)
{
     health -= value;
}

Everything together

The result of applying all suggestions and proper names would be a self-documenting code that no longer has to be commented.

When you write comments you only should explain why you are doing something because the code already shows what.

foreach (var rock in rocks)
{       
    var isPlayerRockCollision = rock.Rectangle().IntesectsWith(player.Rectangle());
    if (isPlayerRockCollision)
    {
        DecreaseHealthBy(Damage.WhenHitRock);
        damagable = false;
        impactHit.Play();
        return;
    }
}

Notice that there is a return inside the if. You set the demagable to false so I think it's not necessary to test other rocks for a hit. You can stop at the first one.

share|improve this answer
    
This should be the accepted answer in my opinion. IntersectsWith() is the method if you need to detect collisions 95% of the time. – denis 12 hours ago
    
Added as accepted answer. It kept getting edited and got better and better. Thanks a lot. – Tyler C 11 hours ago
    
@TylerC yeah, the ideas kept comming so I needed to edit it multiple times - hope it helps ;-) – t3chb0t 11 hours ago
    
@t3chb0t this is perfect, I wasn't sure I was going about it the right way. I appreciate the time and effort you put into this. – Tyler C 11 hours ago

Just a quick answer.

  • You are calculating s.Width / 2 for each Picturebox two times. Just calculate it once, store the result in a variable and use the variable.

  • You are calculating player.Width / 2 twice for each Picturebox which should be done outside of the loop and stored in a variable. The same applies to the calculation of player.Height/ 2. This values won't change unless they are changed inside impactHit.Play().

  • You should check in the if condition first if damageble is true because its faster .

share|improve this answer
    
Thanks for the help, on the third though: If the first condition is false it skips the second calculation? – Tyler C 13 hours ago
    
Yes thats right when using the && operator. – Heslacher 13 hours ago
    
@Heslacher (player.Location.X + (player.Width / 2)) can all be calculated before the loop, same with Y / Height. – EBrown 12 hours ago
    
Likewise, you should test performance between the current code and (int)(s.Location.X * 1.5) (same with Y). – EBrown 12 hours ago

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.