You are here

Chapter 28: Lesson learned #4 - Locating culprits and clumsy mistakes

As I wrote in the previous chapter, the app crashes sometimes if you fire away a few missiles and you get the following screen:

Fortunately, Eclipse gives us a bit more info on this. We can see the whole stack trace, and the exact line of code where the problem occurs:

Apparently, the for-statement fails when it tries to loop through the list of present missiles.

I could simply have added a try-catch here, to pick up any exception so that the app won't crash, but I would really know what happened here and solve the root cause.

I add a few extra debug printouts, so that I know how many missiles there are in the list, and which one we are processing for the moment.

This is what I did in MissileHandler.update() to add debug logging regarding the list of missiles:

public void update(float screenWidth, float screenHeight, int pendingEvent,
   float shipX, float shipY, int shipAngle, List<Asteroid> asteroids) {
  float dX;
  float dY;
  float distance;

  if (pendingEvent == GameEngine.EVENT_FIRE) {
   Missile newMissile = new Missile(bitmap, shipX, shipY, 3, shipAngle);
   missiles.add(newMissile);
  }
  Log.d("JustRoids", "We have " + missiles.size()
    + " missiles in our list");
  int i = 0;
  for (Missile missile : missiles) {
   Log.d("JustRoids", "We will now handle missile #" + i);
   missile.age++;
   if (missile.age > 25 * 3 || missile.collided == true) {
    // TODO: Hardcoded max age. Works better with quadratic game
    // area
    missiles.remove(i);
    missiles.trimToSize();
    Log.d("JustRoids", "We removed one missile and have now "
      + missiles.size() + " missiles in our list");
   } else {
    missile.move(screenWidth, screenHeight);
    for (Asteroid asteroid : asteroids) {
     asteroid.collided = false;
     dX = Math.abs(missile.x - asteroid.x);
     dY = Math.abs(missile.y - asteroid.y);
     distance = (float) Math.sqrt(dX * dX + dY * dY);
     if (distance <= (missile.bitmap.getWidth() / 2 + asteroid.bitmap
       .getWidth() / 2)) {
      asteroid.collided = true;
      missile.collided = true;
     }
    }
    Log.d("JustRoids", "We are now finished with missile #" + i);
    i++;
   }
  }
  Log.d("JustRoids", "We are finished with all missiles during this update"); 
}

Do you see the bug? I found it while adding the Log.d-lines, so I actually did not have to run it. But if you do - be aware of that this logging will make printouts 25 times per second, so you will seriously impact the performance of the app and possible also your computer. Run it just for a short period of time and remove it afterwards.

But back to the bug. Take a look at the same block of code with a bit clearer coloring:

public void update(float screenWidth, float screenHeight, int pendingEvent,
   float shipX, float shipY, int shipAngle, List<Asteroid> asteroids) {
  float dX;
  float dY;
  float distance;

  if (pendingEvent == GameEngine.EVENT_FIRE) {
   Missile newMissile = new Missile(bitmap, shipX, shipY, 3, shipAngle);
   missiles.add(newMissile);
  }
  Log.d("JustRoids", "We have " + missiles.size()
    + " missiles in our list");
  int i = 0;
  for (Missile missile : missiles) {
   Log.d("JustRoids", "We will now handle missile #" + i);
   missile.age++;
   if (missile.age > 25 * 3 || missile.collided == true) {
    // TODO: Hardcoded max age. Works better with quadratic game
    // area
    missiles.remove(i);
    missiles.trimToSize();
    Log.d("JustRoids", "We removed one missile and have now "
      + missiles.size() + " missiles in our list");
   } else {
    missile.move(screenWidth, screenHeight);
    for (Asteroid asteroid : asteroids) {
     asteroid.collided = false;
     dX = Math.abs(missile.x - asteroid.x);
     dY = Math.abs(missile.y - asteroid.y);
     distance = (float) Math.sqrt(dX * dX + dY * dY);
     if (distance <= (missile.bitmap.getWidth() / 2 + asteroid.bitmap
       .getWidth() / 2)) {
      asteroid.collided = true;
      missile.collided = true;
     }
    }
    Log.d("JustRoids", "We are now finished with missile #" + i);
    i++;
   }
  }
  Log.d("JustRoids", "We are finished with all missiles during this update"); 
}

For each active missile, either the yellow block or the green block should be executed; the yellow block for old or collided missiles and the green block for the rest. What I did was two clumsy (but rather common) mistakes.

Instead of using "for (Missile missile : missiles)", I could have chosen "for (int i = 0; i < missiles.size(); i++)", but the call to missiles.size() each loop would have been very expensive. So I introduced a separate integer "i" to keep track of which missile I am on instead. (I need the value so that the "remove" statement in the yellow block can have the correct number as parameter).

The method is pretty ok, but I have two bugs here, marked with red:

  1. I increase "i" only in the green block. It should of course be increased outside. Clumsy.
  2. I do a "trimToSize" during the parsing of the list. The for-loop will end up outside the missile-list's boundaries due to this. Also clumsy.

The correct update-function should be:

public void update(float screenWidth, float screenHeight, int pendingEvent,
   float shipX, float shipY, int shipAngle, List<Asteroid> asteroids) {
  float dX;
  float dY;
  float distance;

  if (pendingEvent == GameEngine.EVENT_FIRE) {
   Missile newMissile = new Missile(bitmap, shipX, shipY, 3, shipAngle);
   missiles.add(newMissile);
  }
  int i = 0;
  for (Missile missile : missiles) {
   missile.age++;
   if (missile.age > 25 * 3 || missile.collided == true) {
    // TODO: Hardcoded max age. Works better with quadratic game
    // area
    missiles.remove(i);
   } else {
    missile.move(screenWidth, screenHeight);
    for (Asteroid asteroid : asteroids) {
     asteroid.collided = false;
     dX = Math.abs(missile.x - asteroid.x);
     dY = Math.abs(missile.y - asteroid.y);
     distance = (float) Math.sqrt(dX * dX + dY * dY);
     if (distance <= (missile.bitmap.getWidth() / 2 + asteroid.bitmap
       .getWidth() / 2)) {
      asteroid.collided = true;
      missile.collided = true;
     }
    }
   }
   i++;
  }
  missiles.trimToSize();
}

These two simple corrections solved the issue!

But if I would have taken the lazy path and just wrapped the code in a "try - catch" block, it would have worked most of the time, but every now and then a missile would behave strangely.

Conclusion, only use try-catch if you know why. Don't use it to wrap bugs - fix them instead.

Theme by Danetsoft and Danang Probo Sayekti inspired by Maksimer