Today, we’ll talk about a bug that shows in practice and 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. Contains 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;
} 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 (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. OR In practice, there should be a check of the Z coordinate: (double)this.Max.Z - (double)this.Min.Z <= (double)num (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. note 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. last line effect 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: Contains 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;
} 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 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: Use table-style code formatting.
Place the similar code in functions.
Avoid redundant operations. For example, instead of type casting of (double)num everywhere, we could simply declare the num variable as double.
Run PVS-Studio static analyzer regularly for additional control. Use table-style code formatting. table-style code formatting Place the similar code in functions. Avoid redundant operations. For example, instead of type casting of (double)num everywhere, we could simply declare the num variable as double. (double)num num double Run PVS-Studio static analyzer regularly for additional control. PVS-Studio

How to Prevent Your Code From Turning Into Sausage That Goes Beyond the Screen

About Author

Comments

TOPICS

THIS ARTICLE WAS FEATURED IN

Related Stories

LLVM 21 Under Static Analysis: Bugs, UB Risks, and Refactor Fallout

106 Stories To Learn About Clean Code

100 Pieces of Programming Advice from the Book Clean Code by Robert Martin

110 Stories To Learn About Refactoring

3 Things Coding And Prose Writing Have In Common According To Cory House

4 Simple Habits For Engineering Teams To Beat Technical Debt

LLVM 21 Under Static Analysis: Bugs, UB Risks, and Refactor Fallout

106 Stories To Learn About Clean Code

100 Pieces of Programming Advice from the Book Clean Code by Robert Martin

110 Stories To Learn About Refactoring

3 Things Coding And Prose Writing Have In Common According To Cory House

4 Simple Habits For Engineering Teams To Beat Technical Debt

Light-Mode

Classic

Newspaper

Minty

Dark-Mode

Neon Noir

Minty

HN StartUps