This week I picked up some code from a colleague for review. To me, part and parcel of the review process is rebuilding the code - I want to check that I get the same hex (i.e., output) file as has been checked in to our version control system for release. And sure enough, this time I did.
But I also got 16 warning messages. The compiler spat out a variety of messages, including warnings of integer truncation, and what looked like notes the developer had written to himself - "This could be done faster with pointers" and the like.
What did this mean? Was I using a different compiler version than the developer? Was my version smarter about flagging potential coding errors? It seemed unlikely, as I got the same hex file. This almost certainly indicated that we were using the same compiler. Also, there was the matter or all those notes-to-self. Those were clearly there for a reason. How did he know it would be faster with pointers? Presumably he'd coded that up to test it, as otherwise he couldn't know that it would be faster? If he had, and it was, why hadn't he used it?
I spent a while scratching my head over this before going to see the developer. "Oh yeah, I had a look at those, the real warnings are all fine. Nothing to worry about. And the others are just reminders to myself.". And the comments about pointers and the like? "It's always faster to do that code with pointers, I just didn't have the time. That's just a reminder in case I look at this again."
Well, that was fine for him, as he'd written the code. But at some point somebody else was going to look at it. Maybe a customer would report a bug, and a colleague would be assigned to fixing it. Maybe we'd want to add some new features. Maybe we'd want to use this code as the basis for another product with similar functionality. Or maybe, as in this case, a reviewer would pick up the code.
In all of these scenarios, Joe Bloggs engineer would pick up some allegedly working code, hit build, be faced with a screenful of warnings, and freeze like a rabbit trapped in headlights. Hampered as he would be by a lack of omnipotence, he wouldn't know that all the warnings are benign, and that a colleague knows a better way to write his own code.
The warnings might indicate real problems. They might be the compiler being picky. But our hypothetical engineer is going to have to assume that each is a land mine requiring individual attention and possible defusing. All of which could have been avoided if the original developer had simply added some casts or whatever else was needed to placate the compiler.
The only acceptable number of warnings in a build is zero. Anything else is going to waste people's time. Any cost saving to the developer in not dealing with the issues as they crop up will be more than offset by the time wasted by a colleague (or possibly themselves) further down the line.
Possibly more importantly, a developer is fooling themselves if they think they can live with a raft of warning messages.
If you have 16 warnings every time you build, you are much less likely to notice the silent arrival of a seventeenth, this one a genuine harbinger of doom. If however your build has always been clean, and is suddenly polluted by a warning - well, you'll see that.
This is indicative of a professional attitude to development, as well as saving time in the long run. If all warnings are examined and addressed as they surface, the ongoing code quality and workmanship are high. If attending to such items is left as a last-minute operation before release, or even worse pencilled in for the often mythical post-release clean-up, then it will either (a) be a huge job, or (b) not get done.
The same comments apply to writing code that is MISRA-compliant, or that gets a clean bill of health from Lint. It's always faster and easier to do this as you go, rather than banging a keyboard for several days and then trying to impose some semblance of quality as an afterthought.
We are what we repeatedly do. Excellence, then, is not an act, but a habit. (I didn't say that, Aristotle did. And according to Wikipedia, it's actually a misattribution. But as misattributions go, it's got to be one of the best.)
No comments:
Post a Comment