Do not let your code turn into sausage that goes beyond screen

Today, we talk about a bug that shows in practice how "code sausage" can cause a series of problems related to the last line effect and careless copy-paste, as well as lead to new errors.

The PVS-Studio team not only creates new diagnostic rules, but also gradually refines the existing ones. For example, we've recently enhanced one of the oldest diagnostic rules in the C# analyzer, V3001, to make it detect redundant parentheses more accurately. As a result, the analyzer started detecting new bugs, one of which we show you.

This case was detected in the Space Engineers project; this is one of the projects in our internal regression testing database. We use a specific old project version to compare how the analyzer behaves on the same code across updates. But if we look at the latest source code, we'll find that the bug is still there. Let's take a look at the Contains function in BoundingBox.cs.

See the problem? Probably not.

Why's that? Because long and indecipherable code lines are developers' foes that should be avoided. It's very easy to make a mistake there, as you can see. Let's rewrite the code a little bit to make it clearer.

public ContainmentType Contains(BoundingSphere sphere)
{
  Vector3 result1;
  Vector3.Clamp(ref sphere.Center, ref this.Min, ref this.Max, out result1);
  float result2;
  Vector3.DistanceSquared(ref sphere.Center, ref result1, out result2);
  float num = sphere.Radius;
  if ((double)result2 > (double)num * (double)num)
    return ContainmentType.Disjoint;
  return (double)this.Min.X + (double)num > (double)sphere.Center.X ||
         (double)sphere.Center.X > (double)this.Max.X - (double)num ||
         ((double)this.Max.X - (double)this.Min.X <= (double)num ||
          (double)this.Min.Y + (double)num > (double)sphere.Center.Y) ||
         ((double)sphere.Center.Y > (double)this.Max.Y - (double)num ||
          (double)this.Max.Y - (double)this.Min.Y <= (double)num ||
         ((double)this.Min.Z + (double)num > (double)sphere.Center.Z ||
          (double)sphere.Center.Z > (double)this.Max.Z - (double)num)) ||
         (double)this.Max.X - (double)this.Min.X <= (double)num ?
           ContainmentType.Intersects : ContainmentType.Contains;
}

Better now, yeah? However, we have to make an effort to spot the error, though. Take a look at the last line of the logical condition:

(double)this.Max.X - (double)this.Min.X <= (double)num

As we can see, it duplicates the third line. The condition is enclosed in extra parentheses, but they're superfluous, as all checks are joined with the OR operator anyway.

In practice, there should be a check of the Z coordinate:

(double)this.Max.Z - (double)this.Min.Z <= (double)num

The analyzer detects it and issues a warning: V3001 There are identical sub-expressions '(double)this.Max.X - (double)this.Min.X <= (double)num' to the left and to the right of the '||' operator.

This is a good example of how a static analyzer complements code review because it's strenuous to manually discern a little typo in such a massive line. We like to call such code "code sausage"—and we've already written a note about how it lures bugs to your code.

The "last line effect" is also shown in all its glory. Typos most often appear at the end of similar code fragments. Technically, we can't talk about lines, since there is a single line. However, the idea still applies: the error occurred in the very last segment of a long, repetitive block.

The bug came from a copy-paste typo. Most likely, developers have copied one sub-expression, pasted it as a new one, and just forgotten to modify it. However, that's not all: this entire line with the error has been copied again, and shows up just a few lines below, in the nearby Contains function:

public void Contains(ref BoundingSphere sphere, out ContainmentType result)
{
  ....
  if ((double)result2 > (double)num * (double)num)
    result = ContainmentType.Disjoint;
  else
    result = (double)this.Min.X + (double)num > (double)sphere.Center.X ||
             (double)sphere.Center.X > (double)this.Max.X - (double)num ||
             ((double)this.Max.X - (double)this.Min.X <= (double)num ||
              (double)this.Min.Y + (double)num > (double)sphere.Center.Y) ||
             ((double)sphere.Center.Y > (double)this.Max.Y - (double)num ||
              (double)this.Max.Y - (double)this.Min.Y <= (double)num ||
             ((double)this.Min.Z + (double)num > (double)sphere.Center.Z ||
              (double)sphere.Center.Z > (double)this.Max.Z - (double)num)) ||
             (double)this.Max.X - (double)this.Min.X <= (double)num ?
               ContainmentType.Intersects : ContainmentType.Contains;
}

It's the same issue with the same warning from the analyzer.

Conclusion

There's no need to go into a long explanation of why this code is problematic, as well as how it should be changed to avoid specific errors. Our readers probably already know that it all comes down to following these tips:

1. Use table-style code formatting.
2. Place the similar code in functions.
3. Avoid redundant operations. For example, instead of type casting of (double)num everywhere, we could simply declare the num variable as double.
4. Run PVS-Studio static analyzer regularly for additional control.