Common subexpressions

I think many of you have seen some developers repeat some common subexpression. For example, it can be something like this:
Point newPoint = new Point(Field1[i+1].Field2.Field3.x, Field1[i+1].Field2.Field3.y));

As you can clearly see, the part “Field1[i+1].Field2.Field3” is repeated twice. Many developers will try to convince you that it is fine because the compiler can inline and remove common subexpressions and, as a result, performance will not suffer.

This statement is correct for almost any language, and it is actively used as an excuse to write this code, and I’ve seen and heard it way more than I wanted. But this code is quite bad, and it is why I always insist on fixing this code. Let me explain why.

Readibility

It requires considerable effort to understand that there is a repeated part, and the second part is exactly the same but reads y instead of x. Moreover, it can create confusion or even lead to bugs. For example, some code before or after may have very similar but still different blocks, for example: “Field1[i].Field2.Field3.x” or “Field1[i+1].Field3.Field3.x”. Somebody can miss the difference and understand the code incorrectly. And then, based on this wrong assumption, they can make incorrect modifications. Then, sadly, these modifications can work until they don’t.

Multiple modifications needed

Every software developer knows that code will eventually be changed. It could improvement or a bug fix. And the developer that change this code may change the first part but forget to change the second part.

I made this mistake myself at the beginning of my career as a software developer. I changed the first expression and was about to change the second. It was something trivial, like changing plus to minus or something similar.  Then my boss came to me to discuss something. When I came back, I thought that I had already fixed the second part and moved on.

Side effects

For example, if the above code is written in C#, then Field3 can be a property that returns some field. Then somebody can change that property to have a side effect, and they may not understand the consequences. I’ve seen it quite a lot of times. Moreover, it may work just fine because there is another bug or side effect. Until somebody else adds another, completely unrelated change and everything collapses.

Perfomance

Field2 can be a simple property that can be inlined. Then somebody adds a little bit of code. Then somebody else adds a little bit of code to property Field3. Now the compiler will not be able to inline them anymore. Then more and more changes. Two years later, the function that contains this expression takes considerably longer to execute, and there is no one to blame because every change was reasonable by itself.

Trust me, I’ve seen it so many times that it is not funny anymore. Most developers are simply concentrating on the task at hand and develop tunnel vision, and they don’t care about anything else unless there are performance tests. But most tests have some tolerance, and as a result, most of the time it will be fine unless they develop performance-critical software.

Debugging

Obviously, if you try to debug in your developer environment, typically it will be possible to calculate any sub-part of this expression. But even the best IDE’s sometimes cannot calculate some types of expressions. Also, sometimes you need to use a much dumber debugger. For example, sometimes I have to use WinDbg, and it cannot easily call functions.

Also, I often have to deal with crash dumps. It is not possible to call functions in the crash dump. But any debugger can show you a local variable. In the worst case, it will be a register.

Other things

Obviously, it takes time for the compiler to try to optimize the common subexpressions. And the faster code is code that never executes. AI also needs more time and tokens to analyze subexpressions.

Attempt to fix

Then finally somebody will try to do something about it. But now it is not easy. Somebody has to plan time for it and create the ticket. Then the developer needs to create a branch, make changes, pass unit tests, and run a build. Then wait until all integration tests pass. And only then merge their changes.

But sometimes that fix will expose other bugs that will require their own investigations and fixes. Depending on the size of the company and the size of the product, it can take anywhere from an hour to a few days. In extreme cases, it can take up to several weeks. Just because somebody was lazy and didn’t like to create a temporary variable.

Conclusion

Please do not listen to other developers about the compiler being smart. A smart compiler is not a reason to write dumb code. It requires very little time to create a temp variable. Do the right thing, and it will save a lot of time for you and your co-workers.

I hope it helps someone.