2D Collision Detection

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.

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.

StackExchange Code Review Q#149782, answer score: 16