How should code reviews be carried out? [duplicate]

Question

My previous question has to do with how to advance code reviews among the developers. Here I am interested in how a code review session should be carried out, so that both the reviewer and reviewed are feeling comfortable with it.

I have done some code reviews before and the experience has been very unpleasant. My previous manager would come to us --on an ad hoc basis-- and tell us to explain our code to him. Since he wasn't very familiar with the code base, whenever he would ask me to explain my code, I'd find myself spending a huge amount of time explaining the most basic structure of my code. As a result, each review would last much too long, and the process would leave both of us exhausted.

Once I was done explaining my work, he would continue by raising issues with it. Most of the issues he raised were cosmetic in nature ( e.g, don't use region for this code block, change the variable name from xxx to yyy even though the later makes even less sense, and so on). After trying this process for few rounds, we found the review session didn't derive much benefits for either of us, and we stopped.

How would you go about making each code review a natural, enjoyable, thought stimulating, bug-fixing and mutual-learning experience? Also, how frequently you do your code reviews - as soon as the code is checked in? Do you allocate a fixed time every week to do this? What are the guidelines that you follow during your code reviews?

Finally, the motivation: What I do want my team to get out of the code review? Answer: I want my team members to be able to cover each other's work. That's the primary motivation. Second motivation: I want all the code to be easy to read/ modify/ extended, so that new features can be added easily.

Answer 1

• Coding guidelines: First I would have formal coding guidelines and tools for an initial check (take out the discussions that are mainly based on personal preference)

• Qualified reviewer: I don't think the manager is qualified to do the code review by default. It should be someone who is familiar with the code base and also has a solid understanding of coding.

• Discussions are an argument in themselves: If you have naming discussions than this means that the name is wrong, it should leave no room for discussion. Agree on a name.

• Open communication: This point is the most difficult it goes to culture and how people communicate. The code-review should be people together trying to improve the quality of the code. It should be open discussions with solid arguments and solid counter arguments and a final agreement. The code-reviewer must be able to clearly state his points, the reviewed must be able to clearly defend his choices and both must be willing to be proven wrong. If you can't make this work let some-one else do the review or think about a code-walkthrough although if the culture is wrong a walkthrough might even be worse.

• Suggestion vs Decision : If the communication is a bit rough than this might be an option. Agree formally that the reviewer can make suggestions, but the decision is up to the reviewed. This sets the scene and gives guidance to both on what to expect.

Hope this helps.

Answer 2

Here are some guidelines that seem to work well.

• Code review every checkin immediately before or after commit. One of the great benefits of code review is that it catches certain types of bugs quickly, before the bugs affect people downstream. Also, the fresher the code is in your mind, the quicker you will be able to address the issues that are raised.

• Focus on the code, not the people. This one can be hard, but everyone should remember that the goal of the code review is to improve code quality, not to show off how much better you are than your coworkers. As an author, recognize before the review happens that your code probably has bugs and/or can be improved. This does not make you a bad programmer.

• Keep the reviews small (200-400 LOC). Any more than that, and reviewer effectiveness is greatly decreased.

• Expect to review 300-500 LOC/hour. If you follow this guideline and the previous one, code reviews should last no longer than 90 minutes.

• Keep managers out of code reviews. Unless you're working in a company where the manager is writing code every day (startups, generally), the manager should not participate. Having the manager participate sets up a bad social dynamic, where the reviewers try to impress the boss and the author works too hard to defend their code.

• Use a tool. Tool-based code reviews are better than meetings in so many ways, but the key (for me) among them is that tool-based code reviews don't require you to block off time on a calendar. My experience is that as soon as time is blocked off, it's used. If the meeting is scheduled for an hour, it will take an hour (or more) regardless of whether the goals for the meeting were accomplished in the first 10 minutes.

Some of these tips come from a free whitepaper published by SmartBear (my employer) on 11 Best Practices for Peer Code Review.

Answer 3

I like the form of code review practiced in most open source projects, which is something like "at least one other person looks at each patch before landing it, and the patch author has to address their suggested improvements." Could call it "patch review."

Kathryn Rotondo has a nice blog post about it here: http://riarockstars.com/2011/02/10/code-review-an-apple-a-day/

I consider patch review primarily a communication mechanism. If you don't do it, then people tend to invent their own coding styles, not know how code they didn't write works, not know about existing code that could be reused, etc. Also the architecture of the overall project tends to be more incoherent without patch review because people don't have to sync up with others.

You could think of patch review as "asynchronous pair programming," perhaps. (One similarity with pair programming is that it helps discourage strong "code ownership" by a single person.)

Secondarily, patch review tends to catch a fair number of silly bugs, and you can enforce rules such as "write unit tests."

An important element in patch review is that patches have to be reviewable. Often, developers treat commits to revision control as sort of an arbitrary snapshotting mechanism, like pressing save in an editor, or to push work-in-progress to the server so they can get to it from another location. These kind of patches aren't reviewable. To be reviewable a patch has to be a single coherent change, well-explained, without "noise" in it. The code itself needs to be comprehensible, too, obviously!

One of the great things people love about git is that it allows you to do the snapshot/save approach as you work, and then later go back and rebase, merging and splitting commits so they are a readable history that makes sense. Then you can submit those patches for review.

If you do patch review for a while, it forces people to write code such that it can be read by someone else, which is huge. Some developers are congenitally unable to do this, and they probably don't need to be on your team.

Patch reviewers should always refuse to approve a patch that doesn't make sense to them or that isn't readable or lacks a good commit message. The idea is the revision control logs and patches should tell a nice, coherent story that makes sense.

Answer 4

Code reviews can be more or less formal. A formal code review takes a considerable amount of time, as it means one thoroughly analyses every single line of code in the given sample. Expect a speed of approx. 200 NLOC (Noncommented Lines Of Code) per hour. Anything faster means one hasn't really fully understood and analysed the code.

In a former workplace of mine, we did formal code reviews in groups of 3-4 (plus the author, if (s)he was available). There was one leader/moderator. To save time, the leader sent around the code sample in question a few days in advance, and everyone was to go through the code and note any issues separately. Issues were classified as major / minor / cosmetic. Then the Excel sheets with the results were sent to the leader, who merged them into a single list. During the review meeting, the group went through the list of major and minor issues (cosmetic issues were found to be trivial and not worthy of discussion in practice). Each item was accepted if it was obvious, or discussed further if there was disagreement over it. Then the author (if available) did the updates based on the final list.

Every meeting took about 1,5-2 hours, and reviewed 300 NLOC on average (note that this was often legacy, spaghetti code - reviewing new code was much faster). This was quite time-consuming, but effective in that we found several major bugs even in old code, and the discussions generated by design / implementation dilemmas were a great learning experience. This is indeed the biggest benefit of formal group code reviews: they help the team to get and maintain a common practical understanding about the accepted coding style and idioms, and to improve the skills and practices of the junior / newer team members quickly.

[Update] This was a legacy project having serious troubles with quality and delivery, so there was a strong urge from management to put order into the chaos. We could review most of new code changes, and a little part of the existing codebase during the year I spent there.[/Update]

In a greenfield project in another workplace, we did much less formal code reviews: for each nontrivial code change, another team member was appointed as reviewer. He did the review on his own, fixed trivial issues and sent the list of nontrivial issues back to the author to be fixed. This was targeted to filter out bugs or issues as effectively as possible. We collected no statistics about its effectiveness though.

I think now that the most effective may be a combination of the two. With formal code reviews, it is very expensive - if at all possible - to fully cover the code in any real-world, non-trivial project. OTOH most of the new code should be already fairly well written (at least after an initial period while the team members understand and agree on the common coding conventions and best practices). So it may be worth to have a single teammate skim through each code change, and arrange a group review only for more complex / critical / interesting code changes.

Answer 5

Code reviews often focus on low-impact minutia, try to focus on the big things

Having been an the wrong end of way too many cosmetic code reviews I generally only look for logic flaws or unsafe/unscalable practices when I go into a review. I sit through 90% of the meeting where people argue about var names nobody is ever going to look at again, usually nobody notices the structural stuff then I bring that up at the end if nobody mentions it.

To my eye 90% of that time is wasted because if the original dev maintains the code he knows his naming, and if a different dev maintains the code he can do a global rename it he is really bothered by the naming. Your mileage may vary.

Answer 6

Scale and scope. In my last organization we had a formal review process. When you had a relatively large change for the quarter (>500 hours), we would have to convene an architecture team for the review. I would have to walk through all of the changes, line by line, and field questions. With changes less than that it would be me, one on one, with the designer, making sure the changes were to specification. Smaller projects were much less formal so as to not waste everyone's time.

Another key skill to this process is learning how to explain complex subjects/processes to non programmers. Einstein said, "You do not really understand something unless you can explain it to your grandmother." I think this also applies to team lead, directors and VPs ;) You learn the art of analogy.

To make the process more enjoyable and less stressful? Don't be confrontational. Stay away from the "I would have done it differently" response. Sometimes the way someone else "skinned the cat" is good enough for what you're doing and you just move on.

Bottom line, we had different types/levels of meetings based on the size and scope of the project.

Answer 7

We use a series of commit hooks, individual repositories and branches to help ensure that reviews kick off after each less than trivial change, as opposed to reviewing an entire sub system overhaul during a two week merge window.

Nobody likes a two hour long meeting where their peers nit pick thousands of lines of code. In an ideal world, you're never reviewing chunks of that size. Sometimes, yes, it is unavoidable but more often than not reviews can be done in smaller bites. The last thing you want is being told "re-factor all of that" a few days before a deadline. Those sort of problems can be caught much earlier.

We have a few people who have keys to central repositories that eventually turn into production builds. People working on various projects send pull requests (very similar to how the Linux kernel does it) asking for new code to be pulled into the blessed repository. Additionally, every push to an individual repository kicks off some hooks that:

• Possibly update a bug in the bug database
• Send e-mail to those that need to be informed
• Launches several lints

In some cases, a build might also be triggered.

This lets everyone else working on the same team (including managers) take notice of possible issues immediately, and reviewing them is as simple as replying to the e-mail that is generated.

The final review comes at the pull request, but by that time, most of the code has already been reviewed. Remarks at the final review are usually something along the lines of:

• "I need that documented before I can pull it, or we'll just forget to do it later"
• "You don't need to work around [compiler bug] any more, we switched to [new version]"
• "Joe implemented some static functions that almost duplicate your's in [area], maybe you two can collaborate and make a single shared library"

But, seldom are the issues at this point related to the code that any given person wrote, it's mostly just issues in merging everything together.

Answer 8

A lot will depend what the code is needed for but whilst reviews will lead to more perfect code, it uses up your resources and can severely delay the development procedure.

Reviewing when the developer has spent their brain cells working out the best solution for the issue, worked through all the issues of getting it to work to spec fully tested, the scope for change is relatively small, and any large-scale change would certainly indicate that the developer has wasted too much time on the wrong path for too long and that this should have been addressed earlier.

What Joel suggests is "hallway usability testing" whereby you show your code to a peer who might have to use it and they have a look to see if they can comprehend it very quickly.

So you have to weigh out the balance, particularly as the successful business is likely to be the one that releases early. Review the path relatively early, hallways usability testing? yes. Minor modifications afterwards? yes. Major walkthrough requiring "senior" developers scrutinising every line? No, unless your coder is a junior keen to learn.