In a code review a developer needs to look at the code from two different perspectives:
- Correctness. Is the code logically correct, does it do what it is supposed to do? Will it hold up in the real world? Is it safe? Does it handle errors and exceptions? Does it check for bad input parameters and return values? Is it secure? And where performance is important, is it efficient?
- Maintainability.Can I understand this code well enough to maintain it, could I change it safely myself? Is it readable and consistent? Is the logic too complex, are the pieces too big? Is it unit tested and if not can it be unit tested? This is where a reviewer looks out for too much copying and pasting, whether hand-rolled code could be done using standard libraries or language features instead, adherence to style guidelines.
It’s obvious that using good names for classes and methods and variables is important in making code understandable (if you can’t understand it, you can’t tell if it is doing what it is supposed to do) and easier to maintain. Books like Clean Code and Code Complete have entire chapters on proper naming. But even good, experienced developers have a hard time coming up with the right abstractions and good, meaningful, intention-revealing names for them. It’s especially hard if they’re working on code that they don’t know that well.
Bad names cause reviewers to stumble, or to make the wrong assumptions about what the code is doing. There are lame names – lazy, ambiguous, generic names that don’t help the reader understand what’s happening in the code. Then there are names which are misleading or just plain wrong – names which used to be right, but aren’t now because the logic changed but the name didn’t. You’re reading through code, it calls postPayment, but postPayment doesn’t just post a payment, it does a lot more now – or worse, it doesn’t post a payment at all any more.
Focusing on naming becomes more important over time as more code gets changed more often. The design changes, responsibilities change, often incrementally and subtly. Code gets added, then later other code gets deleted or moved. The programmer making the latest change just wants to focus on what they’re doing, fix the code and get out, and doesn’t look at the bigger picture, doesn’t notice that the meaning of the code has changed or become less clear because of what they have just done. Reviewers need to be on the watch for these kinds of problems.
But naming isn’t just about making it easier for somebody else to read the code and maintain it – or even making it easier for you to read it and change it yourself when you come back to it later. As a colleague of mine has pointed out, if someone can’t come up with a good name for a method or class, it’s a sign that they are having problems understanding what they were working on. So in reviewing code, a bad name is more than just a distraction or an irritation – it’s a warning sign that there could be more problems in the code, that the developer might not have understood the design well enough to make the change correctly, or that they weren’t paying close enough attention to what they were working on, and they may have made other mistakes that you – the reviewer – need to be on the lookout for.
Focusing on naming sometimes seems fussy and nit picky, another battle in the endless “style wars” which end in hand-to-hand fighting over bracket placement and indentation. But good naming is much more important than aesthetics or standardization. It’s about making sure that code is working correctly, and that it will stay that way.
Reference: Code and Code Reviews: What’s in a Name? from our JCG partner Jim Bird at the Building Real Software blog.
