In my last post, I made a case that code reviews cannot be relied upon as the main activity to achieve defect prevention, correctness, robustness, usability, or learning. Even so, these goals are worth pursuing in code reviews, though other practices such as unit testing and QA should be the primary driver to such goals. Aside from those goals, code reviews can indeed be the primary driver of other areas of improvement for a software team. It is arguable (comments welcome), but I say above all else code reviews routinely and consistently improve code readability. Highly readable code might not be the first measure of software quality or of team competency that you'd list, but it should be. Code is typically written once and revised a few times, but read dozens of times. Programs are for humans to read and only incidentally for computers to execute. I or any competent developer can deal with any code that we can understand, and the quicker we can understand it, the faster it will be to troubleshoot, maintain, and enhance. Readable code gives us more than a fighting chance. Too often, when teams are under the gun and avoiding spending any time or effort into code review, we get indecipherable, weird, arcane, and confusing code, and this incurs troubleshooting, maintenance, and enhancement costs to go way up in the future, and in unpredictable ways.
The most important imperative in generating highly-readable code, I say, is: write your code so that the reader can immediately and intuitively discern what it actually does. Don't make me think too hard. Make the code obvious. As a reviewer, you can make this your top priority when recommending changes to the code under review. A reviewer ought to first point out where she or he doesn't understand what the code is doing and try to collaborate with the author to make it obvious. As an author, when a reviewer doesn't immediately grok what you wrote, your first thought shouldn't be "Okay, let me explain it to him" and certainly not "I should probably add some comments that explain this". Instead, think "If he didn't get it, others won't get it either. What code should change to make this more obvious?"
More often, the reviewer already understands what the code is doing but quickly thinks of a better way to express it--"better" here being synonymous with "more readable". Code review is the only practice I know of that presses code into a more readable state routinely and reliably. These can often be the quick wins: the reviewer says "try this instead", and very often the author can quickly agree and integrate such a change. As an author, I am very apt to take my coworker's suggestions, certainly when I am ambivalent and even when I mildly disagree. The rapport and mutuality gained from adopting a reviewer's recommendation in code review might be more valuable than the change itself. And several times I have looked back on such changes after several months and been convinced in the end--turns out my teammates are really quite good at this programming stuff!
We should all write our code so that the mapping from the code itself to our intent is as simple and as straightforward as possible. For example, if I intend for a method to calculate and then save, naming it CalculateAndSave is better than naming it Save. In a code review, pointing out "this is doing more than saving" will at least result in a renaming of the method. Then, that name then might lead us to question if the calculation should be done in a separate method for better separation of concerns. Conversely, if I named a method ValidateAndSave but there is no validation code in the method at all, my teammate would immediately point out that I intended to validate but never actually did so. Pointing out the differences between apparent intent (or mutually-understood requirements) and actual code is a top benefit of doing code reviews.
So what makes code highly readable? Sometimes "it depends" (of course). Sometimes, shorter code is more readable (e.g. omitting redundancies), and sometimes longer code is more readable (such as declaring a semantically meaningful temporary reference). Sometimes clever code is more readable (i.e. when your cleverness came up with the apt statement; please not when cleverness is needed to understand it); sometimes "dumb" code is more readable (especially when familiar and unastonishing). Sometimes more abstraction (more layers) is more readable; other times, collapsing layers makes it more readable. When these questions come up in code, the important thing is that the team is deciding together which way to go in each case. If the team is skilled and genuinely cares, I trust them to make a decision, even if I disagree with that decision. The difference between my idea of good code and my team's idea is usually tiny compared to code that isn't reviewed for readability at all.
However, there are some good readable-code rules-of-thumb that are almost always true. Simple beats complex. Boring beats exciting. Code written as others expect beats astonishing code. Conventional code beats innovative code. Pattern-following code beats trailblazing code. Linear code beats nested code, and shallower nesting beats deeper nesting. More short methods are better than fewer long methods. I think we all appreciate simple, boring, as-expected,
conventional, patterned, linear, shorter code--so as a code reviewer, be eager to give your recommendations to make it such.
And there is one more rule-of-thumb that I hold to be the most important: refactored code beats original code every time. Code can almost always be made more readable by being revised, especially when there is no pressure to change its behavior. This is exactly where team peer code review shines: It allows for code to be refactored mere minutes or hours from when it was originally written, with zero added cost of extra testing, when you are most familiar with both the desired and actual behavior of the code in question, when such refactoring is typically least costly and most welcome by all concerned parties. It is the biggest no-brainer when it comes to code review: seize the opportunity to refactor your code to be more readable, and leverage code reviews to spur changes for readability.
Of course, even if all that is agreed among team member, opinions will still differ as to what is easiest to read. One thinks nested ternary conditionals are easier than using elseif; their teammates think they're nuts. Some like indenting via tabs, some like spaces. When these differences are aired in a team peer code review, a quick resolution is likely. If the teams uphold the value of others' work and want to come to a consensus, a disagreement in a code review will result in the team either proposing a common practice that all can and should agree on, or it will result in the team agreeing that either way is fine and toleration of unimportant inconsistencies (if any). The important concern is that the team be empowered to consider and decide themselves, and that any team member can speak up to raise the issue (both within the code review and in initiating a broader team discussion).
Making the code more obvious and making the code reflect the intent of the author will result in big long-term wins for the product and for the team. Something curious happens once a team gets a little experience at this: they inevitably start forming team norms that lead to more efficient and productive coding. Often it also leads to agreed coding standards that take further time-consuming discussions (or arguments, or fights) out-of-play. These benefits result in snowballing productivity and team happiness as well, so I'll explore that in my next post. I've only scratched the surface on some other benefits of code review, which will also be coming up.
Do you have a piece of code you'd like to make more readable? Post it here (maybe a link to a GitHub PR or commit?) and I'll give it a crack.
No comments:
Post a Comment