Code Review
Writing software is a skill that can be acquired and honed with practice. It can indeed be artful, and some people have an uncanny talent for it, but it is still a science. And just as a good novel is the creative endeavor of its author, a solid grasp of language and adherence to syntax guidelines are what make it readable.
Today's programmer must be conscious of governance, security issues, the old-fashioned notions of quality, and the ultimate business question: Is this software doing us some good? Thus, a certain amount of our attention must focus on testing and review. Test a program to see what it does. Review it to see how it does it.
Often testing is performed in a 'black box' style, meaning that the person executing the software is purposefully not aware of the mechanics, just the resulting behavior. The downside of this? After fifty years we have all seen MultiValue [ and non-MultiValue - Ed ] solutions that work great, but when you take a peek under the hood, Italian food comes to mind. Spaghetti code. And who hasn't stepped in as a contractor or a new hire and had to slog through a bunch of poorly written and inconsistently maintained software? Code review can be the cure.
There is a difference between code review and peer review, but in many ways they are the same. For one thing, in either case you want to have someone else do the reviewing; not the person who did the coding. There is nothing like a fresh pair of eyes! Peer review may be more casual and often includes a bit of unit testing, where code review, by its name alone, is about looking inside the box at the actual mechanics of the software: The source code.
In both cases it is convenient to have a checklist; and in both cases it is important to keep the checklist fairly short and flexible. Code review tends to be the more rigorous of the two processes. It is often performed by individuals who are dedicated and have, shall we say, the personality for it.
Jeff Atwood is an American software developer, author, blogger, and entrepreneur. He is known for the programming blog Coding Horror, and is the co-founder of the question-and-answer websites Stack Overflow and the Stack Exchange. Most of us know who he is; many of us respect his opinion. He recently voiced his view on this topic:
"Peer code reviews are the single biggest thing you can do to improve your code."
What is the purpose of code reviewing? To catch bugs. To insure that code is readable and maintainable — and fairly standard. To spread knowledge of the code base throughout the team as well as to get new people up to speed. It is also great for exposing programmers to different approaches and often little nuggets of wisdom. How often have you looked over another programmer's shoulder and said "I didn't know you could do that!?"
If code review is so great, why doesn't every development group have a code review stage in their lifecycle? Everything boils down to the idea that there simply isn't time . That old chestnut about a stitch in time saving nine merely inspires a snorted "prove it!" We probably can't — not on a specific basis — until the practice is implemented and metrics are reviewed. Plus, programmers resent testers, and perhaps code reviewers even more, because they (we) don't want another programmer — a peer, no less — to be looking over our work and possibly criticizing it. Yes, it's really just another pair of eyes helping you make sure your work reflects your best effort… but it feels like judgment. And hassle. We resent the idea of code reviewers having authority.
In many cases that resentment is justified. Who wants a petty tyrant running the code reviews? There are ways to relieve that pressure, of course. One way is to create a guideline or checklist, but make sure that it is not onerous. And be sure to allow it to evolve with feedback from all parties.
What might a code reviewing checklist include? The core imperative is to organize complexity. Source code should be clear and readable. The intention of the program should be revealed by looking at its source. Here are some starting points:
- Source code should be clean. One important way to achieve this is by deleting — not commenting out, deleting — obsolete code. There may be a valid reason for keeping a remarked section but it would usually be temporary. It should not be standard practice. Your source code control software will manage the change (and if it doesn't, give us a call).
- There should be no repeated sections; no copy-and-paste code.
- Boy Scout principle: See that the code is in a better state than before this change.
- Check for debugging clauses and temporary remarks; reminders that programmers put into the source code, but then forgot to go back and address.
- Optimize if you can. If you know a more efficient way to accomplish something, share with the programmer.
- Check for standards. Does the new or changed section of this program match the previous architecture? Does it meet generally accepted standards ? Does it meet our company standards ?
- Note that in order for the latter two to be considered you may want to spend some time documenting the company standards and make information about generally accepted practices available.
- Some generally agreed upon standards both within and outside of Multivalue are
- No (or limited) GOTO
- Module re-use with GOSUB
- Minimize multiple and nested returns
- Numeric vs word labels
- Establish naming conventions for variables, programs, and other parts of the software.
- Variables
- The most important thing is to keep variable names unique. Don't use short variable names that may get reused — and their value reset — in other called subroutines.
- On the other hand, don't get carried away with a lengthy set of rules for naming variables - Google Hungarian Notation Abuse for some amusing side effects of this effort. One example: Avoid prefixes that have contradictory meanings, are inconsistently used, and end up making the variable names unwieldy with no real benefit.
- Programs and other components benefit from the same guidelines, along with the additional consideration of how pronounceable the program name is. Imagine a conversation where you have to hiccup every time you want to refer to a particular routine.
- Casing: Uppercase, lowercase or camel case. We all have our preferences, but please let's agree on something before we create a veritable quagmire of ASCII.
- Testability: Is the program ready to be tested?
- Is there a framework for a test mode? For example, are there sections that display pertinent progress information if a debug flag is set?
- Should your code review include reviewing against a test plan? Or reviewing the test plan itself?
- Can any test code be replaced with the use of an existing API?
Once you've developed some standards it is possible to at least semi-automate some of the code reviewing functions. In PRC (gratuitous plug), a set of standards can be applied to code during the process to mark a project "development complete." Any failure to adhere to the automated standards can prevent the project from moving forward to the test phase, or whatever is next in the lifecycle.
This checklist is only a starting point. The key is to allow the checklists to evolve. Let people add, delete and refine. With their feedback also comes buy-in.
Armed with a great checklist, code reviews can be simple and quick while helping you drive up coding standards and avoid inconsistent quality.
