An exercise for the reader - what is wrong with this code?

excerpt.java

  public void doExplode()
  {
    if (!world().isRemote)
    {
      for (int x = (int)-getRadius(); x < getRadius(); x++)
      {
        for (int y = (int)-getRadius(); y < getRadius(); y++)
        {
          for (int z = (int)-getRadius(); z < getRadius(); z++)
          {
            Vector3 targetPosition = this.position.clone().translate(new Vector3(x, y, z));

            double dist = this.position.distance(targetPosition);

            if (dist < getRadius())
            {
              int blockID = targetPosition.getBlockID(world());
              Block block = Block.field_71973_m[blockID];

              if ((block != null) && (!block.isAirBlock(world(), x, y, x)))
              {
                if ((this.destroyBedrock) || (block.func_71934_m(world(), x, y, x) >= 0.0F))
                {
                  if ((dist < getRadius() - 1.0F) || (world().field_73012_v.nextFloat() > 0.7D))
                  {
                    targetPosition.setBlock(world(), 0);
                  }
                }
              }
            }
          }
        }
      }
    }
  }

No no, this code extract does not define getRadius(). My point is that reading the code, you would think maybe getRadius() will be a double and a fairly low number like 4.62D or something. The discovery that it is (a) a constant value, (b) an integer, and (c) as high as 36, makes the code look very different.

getRadius() returns a double precision value which is 36.00000000000000D. The code then has to do the work of casting that to an integer. Casts are expensive, in performance terms, because the JVM first has to check that the double value is within the bounds of a legitimate integer value. Because getRadius() is a method call, the JVM has to do that cast every time - it can't simply do it once and assume that getRadius() will return the same value in future.

(OK maybe some JVM implementations have runtime optimisations that would eventually replace the method call with an in-line value, but a programmer can't be sure how a JVM runtime optimisation will work, and in my experience + testing it does not actually do that.)

Even worse, the way that Java compiles a for() loop into bytecode results in the condition part of the loop being re-evaluated every iteration through the loop. So in this line:

for (int z = (int)-getRadius(); z < getRadius(); z++)

the part which reads "z < getRadius()" has to be evaluated 373,248 times, or more accurately 378,432 times as the condition also has to be evaluated on the final attempt to iterate through the loop which fails because the condition is failed.

Assuming that a Minecraft mod should be coded for performance - as seems obvious, to anyone who has ever run a server with multiple mods - this code from ICBM just makes me want to bang my head against a wall, it is so bad.