Checking whether a bullet has hit a Space Invader

Question

I just (almost) finished a Space-Invaders game in Javascript. I have a few bugs to clear out of the way but meanwhile I wanted to ask how I can improve the method I use to test if one of my 'Bullets' has killed an 'Invader'.

I have a feeling I used too many loops and there must be a more elegant way.

The whole program may be found here.

The exact function is:

function testForInvaders(bullet){
    var score = document.getElementById('score');
    var bulletX = bullet.getLocation()[0],
        bulletY = bullet.getLocation()[1];

    for(var i = 0; i < Invaders.length; i++){
        var invaderX = Invaders[i].getLocation()[0],
            invaderY = Invaders[i].getLocation()[1];

        if(Invaders[i].isVisible()){
            // if bullet is in the invaders radius...
            if(bulletX >= (invaderX - 15) && bulletX <= (invaderX + 15) &&
            bulletY >= (invaderY - 15) && bulletY <= (invaderY + 15)){
                Invaders[i].removeVisibility();
                game.incrementScore();
                game.updateScore(score);
                return false;
            }
        }
    }
    return true;
}

This function is called from within my 'loop' function which is the interval the draws all elements to the canvas. The logic is as follows:

• move all invaders (loop through them)

• move all bullets (loop through them)

• for each bullet I call the above function and test against all Invaders.

• then I loop through everything once more to draw it to the canvas.

How can I improve this? General feedback would be welcome as well :)

Answer 1

Welcome to CodeReview!

I'm not really working with javascript, but I noticed some things I'd like to mention:

• You can safely move the first two initialisations of invaderX an invaderY one condition deeper into the Invaders[i].isVisible() block - if the invader isn't visible there is no need to know the invaders position.
• Same with the score variable. Initialise the variable immediately before using it (game.updateScore(score)).
• Don't use magic numbers like 15 for the radius of an invader. Use a static class constant or instance property of an Invader instance. This would even allow you to create invaders of different size.

• I would also put the isInInvadersRadius condition into a separate function. This function could look something like this:

  function areInRadius(location1, location2, radius) {
      var x1 = location1[0],
          y1 = location1[1],
          x2 = location2[0],
          y2 = location2[1];
  
      return x1 >= x2 - radius && x1 <= x2 + radius &&
             y1 >= y2 - radius && y1 <= y2 + radius;
  }
  

  Using this function you wouldn't even need the temporaries bulletX and bulletY, just calling areInRadius(bullet.location(), Invaders[i].location(), Invader[i].radius).

  Your function would look like this:

  function testForInvaders(bullet) {
      for(var i = 0; i < Invaders.length; i++) {
          if(Invaders[i].isVisible() && areInRadius(bullet.location(), Invaders[i].location, Invaders[i].radius) {
              Invaders[i].removeVisibility();
  
              var score = document.getElementById('score');
              game.incrementScore();
              game.updateScore(score);
              return false;
          }
      }
  
      return true;
  }
  

• testForInvaders gets called quite often as you already mentioned and I'm honestly not sure if the overhead of the function call above will screw with the performance. You should consider to measure the impact.

Since I'm not into javascript I can't say anything special about hit detection (maybe there is a builtin or an standard way of doing so), but I'm sure other users will remind you of those (if existent).

Nice piece of code you delivered here for your first review. Keep it up!

Answer 2

The basic change I would make with this code would be to brak it up into smaller functions, i.e.:

function testForInvaders(bullet){
    var bulletX = bullet.getLocation()[0],
        bulletY = bullet.getLocation()[1];

    for (var i = 0; i < Invaders.length; i++) {
      var hit = testForInvader(bulletX, bulletY, Invaders[i] );
      if (hit) break;
    }
 }

 function testForInvader(bulletX, bulletY, invader ) {
   if (invader.isVisible() && didCollide(bulletX, bulletY, invader) ){
      invader.removeVisibility();
      game.incrementScore();
      game.updateScore();
      return true;
    }
    return false;
  }

  function didCollide(bulletX, bulletY, invader) {
    var invaderX = invader.getLocation()[0],
        invaderY = invader.getLocation()[1];

    // if bullet is in the invaders radius...
    return (bulletX >= invaderX - 15) && (bulletX <= invaderX + 15) &&
           (bulletY >= invaderY - 15) && (bulletY <= invaderY + 15);
  }

Some notes:

• Coding guidelines for JavaScript generally recommend starting variables with a lowercase letters so name your variable invaders not Invaders.

• Instead of using an array for you location and accesing it as location[0] rather return an object. This can be as simple as a hash {x: 12, y:23} or you can use a more complex object with functions but it would make the code simpler to read and make it easier to pass parameters around.

• I would get the score element inside the updateScore method or, more likely, store it in a variable somewhere when I created the element or initialized the game. I would also have incrementScore automatically update the score display in most cases.

• Rather than loop through each bullet and check if they have hit an invader I would loop through invaders and check if they have hit a bullet. I'm not saying this is right or wrong, just a hunch from having written this kind of code before.

• As a further step, and as you learn more, I would consider turning your invaders and bullets into classes and objects. This is a fairly advanced topic butI just saw someone ask for a review on something similar: JavaScript canvas bouncing objects