During past few weeks, I've been reviewing an old codebase. Some functions were in place since 2008. You may think that those functions are bug-free. After seven years of usage, every issue should have emerged.

But the story is different. The number of errors I've found is impressive. Memory leaks, files left open, checks on conditions that can never be true (see also the last Horror Code), and, worst of all, logical errors. What do I mean with logical errors? Let me give you an example.

A Logical Error

There is a file descriptor declared as a global variable (OK, this could be considered a logical error too, but please keep reading) and a function that does some elaborations and then writes the result in a file descriptor. Among the parameters of this function there is a file descriptor... unfortunately it is never used. The writing is done on the global variable.

Everything works fine just because the global file descriptor is the only one used in that program. What if in the future someone would have used that function passing a different fd? How long it would have take to find the bug?

By the way, the compiler has signaled with a warning that a parameter of the function was not used but nobody cared. You should always take care of warnings!

Conclusions

A code review is always a good thing to do. Probably you won't find big bugs, but surely the stability and the quality of your software will be improved. And maybe that strange situation so difficult to reproduce will never be reported again.

Image credits: Nice to meet you! by Predi licensed under CC BY-ND 2.0

Post last updated on 2019/01/05.