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);
                  }
                }
              }
            }
          }
        }
      }
    }
  }

Question B. Estimate how many times the inner loop (from Vector3 targetPosition = ... to the end) is called?

Question C. Explain why the author uses double-precision floating point arithmetic in Vector3?

Question A. What do you think getRadius() returns?

How would I know? getRadius() isn't declared anywhere in this piece of code, probably it's a radius of a sphere, which would be a double, presumably.

Question B. Estimate how many times the inner loop (from Vector3 targetPosition = ... to the end) is called?

The integer of radius, 2 times, plus 1 (if radius is not equal to (int)radius), and then cubed.

Question C. Explain why the author uses double-precision floating point arithmetic in Vector3?

Because I need floating point precision coords to measure the distance between two points. Otherwise, if the distance between the targetPosition and this' position is less than 1.00, it may return zero even if both points aren't at the same coord.

This code is from Calclavia's ICBM mod, the antimatter explosion code. Some of these are trick questions...

Correct answer to question A: getRadius() returns an integer, and it's the same integer: 36 every time.

Question B you have correct. If getRadius() is 36, that's 373,248 times.

Question C, the answer is for no good reason at all. These are all block coordinates which are integers in Minecraft, so integer arithmetic could have been used throughout.

There are so many other things wrong with this code, it's hard to know where to begin. In a Minecraft mod any loop which runs 373,248 times is absolutely performance critical - if it is not optimised, the server will LAG badly or even produce stack overflows and crashes.

This results in endless discussion between server admins / modpack creators / other non-coders (e.g. take a look at FTB forums) about how to cope with a lagging server, plugins to disable antimatter explosives etc. Instead of the obvious answer: fix the bad code.

Okay, at least I got a 3/10.

But wait, you said getRadius() returns an integer, so what's point having (int) in (int)-getRadius()? Like in Python, I use int(num) to convert a float into integer (and minus to reverse the sign of a number as well). If the variable is already an integer, there is no point using int(num).

Besides, how do you tell getRadius() is 36 in that code? Or I'm so Java-illiterate I can't spot.

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.