Funmurphys: the Blog

I am a big fan of code review! Whether the software is written in Java, C++, Python, or (shudder!) Fortran, code review serves a number of important purposes in an organization that develops software. When I worked at Smarttalk.com we used a system of code review designed to produce high-quality code quickly. Here were the steps:

1. When a developer thinks he/she is done with a single code module (one source code file), that developer calls for a code review. The broadcast e-mail and meeting invitation must go out no less than 48 hours before the scheduled review, so the other developers have at least 48 hours to look over the code before the meeting. You can schedule a code review for 3:00 pm on Thursday as long as you click on the Send button by 2:59 pm on Tuesday (and there were a few of those).
2. The software developer can invite anyone they want to the review, but at least one participant must be at the Senior level. Go ahead and invite your friends! There was some worry about people only inviting people who would be favorable to them, but we quickly learned this principle: Friends don’t let friends write bad software. You are more likely to accept constructive criticism from your friends than from people you don’t get along with.
3. A quorum is three people: the developer and two other software engineers, one of whom must be at the Senior level. The code review works better with about 4-5 people.
4. The code file to review must be less than 1,000 lines long. Generally the other developers retrieve the code from the repository.
5. The meeting will not last longer than 1 hour. If the code review runs overtime, the participants adjourn and re-schedule.
6. The goal of the code review is to improve the code. All our jobs depend on producing and marketing high-quality software, and it is in everyone’s best interest for the code in the repository to work well and be easily maintained.
7. Everyone is nervous at first about their work being reviewed, and for this reason there is a Moderator. The moderator makes sure everyone stays on track, watches out for criticism that is not helpful, and ensures that the experience is professionally positive for the developer. The moderator has the power to kick someone out of the meeting if they are being arrogant or derogatory. I never saw that happen, though – usually it is enough to remind someone that the goal of the code review is to improve the code.
8. Everyone at the review sits around a table with a listing in front of them. The listing is printed with line numbers for easy reference. The developer begins with a brief explanation of the purpose of the code.
9. The developer leads everyone else through the code, page by page, or subroutine by subroutine. Everything is fair game for improvement. The reviewers speak up in they have comments on a certain section. If not, the review moves on.
10. It is great for less experienced programmers to attend! They can learn a lot from someone else’s code. Often they make very worthwhile contributions by requesting more extensive comments for some algorithm that is not clear. Sometimes you get people who just watch out for uniform spacing or lines longer than 80 characters. Those people contribute something positive to the process.
11. Everyone looks for adherence to the coding standards.
12. Sometimes design flaws are caught in a code review. The script has to sanitize input for web security. The code has to handle Unicode characters. Sometime a subroutine is just too long!
13. Reviewers can suggest tests that might produce a failure, if they suspect a bug. The developer makes a note of those and will verify later that the software operates correctly, or fixes the bug if it does not.
14. If extensive changes are requested, then another code review will be scheduled when the first round of changes are complete. This is rare. Even if there are lots of comments, usually the revisions take only a few days to complete.
15. After the review: When the developer completes the requested changes to the software, he/she adds a note to the header comments of the file saying that the code was reviewed on that particular date by the following people (full names). Then the developer may check in the code, and move on to another file.

The developer under review gets completed feedback in 49 hours. I’m sure our loyal Funmurphys readers can add to this process and improve it. Tweak it for your organization. Remember that the primary purpose of code review is to improve the code. A secondary purpose is to provide on-the-job training for the more junior programmers. From time to time people will see a clever way of doing something, or suggest one, and notice code overlap that can be eliminated.

Those should be reasons enough for any professional software developer to conduct code reviews. But suppose you are out there thinking, “I’m a good engineer! I’ve written code for years, it’s well tested, and there are only a few minor bugs in my work. I’m good! I don’t need code review. It’s just a waste of my time.” Yeah, you’re kind of arrogant, but you have a right to be.

Here’s why you should request and schedule code reviews on your own code. Because someday, when you least expect it, someone else will come along and have to maintain or extend your code. And yeah, they won’t be as smart as you. They won’t understand your brilliance. They won’t take the time to read through your code and figure out how it’s carefully designed to work. What they will do is go to your boss and say the following: “This code that YourName wrote is dog poop! (but they won’t say ‘poop’)! I can’t follow it at all. This code has to be completely re-written!”

Then your boss will be faced with a dilemma. She knows you are a good software engineer and you write good code. She has not heard any problems with your code until now. But the new person is so insistent that your code is dog poop, that it has to be thrown out and re-written from scratch, and the new person uses lots of CAPITAL LETTERS and exclamation marks and derogatory language in his e-mail complaint. I have seen this happen. What should your boss do?

Your boss has to evaluate the complaint. Your boss has to examine the code you wrote with a fine-toothed comb, checking if you are correct or if the new guy has some validity to what he says. Your boss has to call a technical review at which your detractor will be the star witness. You can’t defend yourself very well since you have moved on to another part of the project. You’re under suspicion. Unless . . .

Unless there is a short section in the header saying the following: This code was reviewed on March 28, 2010 by the following people: Ashley Adams, Bob Biltmore, Charlie Chang, and Debbie Dumas. When your boss finds that comment in the code, her course of action is simple: “Sorry, Mr. New Guy, but this code was reviewed by the software development team and found to be acceptable. You can suggest some improvements, but we are not throwing it out and re-writing the whole thing from scratch. If you cannot understand some else’s code, we will send you back into circulation and hire someone who can.”

Yes, code reviews can be held to Cover Your Anatomy. It’s not a great reason to do them. If the good reasons above are not sufficient to urge you to do code reviews, perhaps this bad one will. Enough said.

Now let’s turn back the clock 400 years to post-Elizabethan England, during the reign of King James I. He decided to produce a new translation of the Bible, the Authorized Version that would bear his name. Alister McGrath has written a great book about this period, which you should stop reading this blog to go out and buy, then come back here. The book is “In the Beginning: The Story of the King James Bible and How it Changed a Nation, a Language, and a Culture” (2001). The translators worked in teams. On page 187 we read this account of the actual translation process:

The translation in King James’ time took an excellent way. That part of the Bible was given to him who was most excellent in such a tongue (as the Apocrypha to Andrew Downes), and then they met together, and one read the translation, the rest holding in their hands some Bible, either of the learned tongues, or French, Spanish, Italian, etc. If they found any fault, they spoke up; if not, he read on.

When I read this excerpt I laughed out loud. These guys are doing a code review! They used the same process 400 years ago to produce a good translation of the Bible that we use in modern times to produce good software! They all sat around a table and checked up on each other’s work. When the King James translators found a discrepancy, they worked together to resolve the problem That’s just what we do, too! I am sure that nobody at Smarttalk or any other software company set out to emulate the King James Bible scholars. But we have hit on the same process to produce a high-quality product.

This does not mean that the King James Bible is a perfect translation; the McGrath book documents a few errors that were corrected later. Another example is Hebrews 11:3 in the King James Version, which reads as follows: “Through faith we understand that the worlds were framed by the word of God, so that things which are seen were not made of things which do appear.” But the Greek word used for ‘worlds’ there is Aion (Strong’s Concordance G165), and should be translated into English as ‘eons’ or ‘ages’. God’s word of command caused huge spans of time to come to pass, as well as physical worlds. The KJV is not perfect. Nevertheless, if people are still using my code 400 years from now, my descendants should be very proud!

Maybe someday I will compare and contrast the process of code review with the peer review conducted by scientific journals.

Tags: Bible, C++, code review, Faith, Java, King James Bible, peer review, software, software engineering