Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

This question already has an answer here:

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.

share|improve this question
    
I will attempt to answer your question, in order to do that I need some context, what do you want your team members to get out of the code review? My only advice until I get time to answer your question in detail would be that, a developers peers should be reviewing the code, and you should help the developers on your team do that. I can tell you from experience as a new developer on a project, and the user just "expects it to work", and if it doesn't then that is a major problem Code Reviews do work if they are done correctly. –  Ramhound Feb 18 '11 at 14:41
    
@Ramhound, question updated to address your comment. –  Graviton Feb 18 '11 at 15:51
1  
Might have been nice for your manager to try to understand the code before wasting your time. I'd review the code separately and then come back with queries and comments, and ask for fixes if required. Then repeat the process, ad nauseam. –  Alison Feb 18 '11 at 16:10
add comment

marked as duplicate by gnat, BЈовић, Kilian Foth, Ryathal, MichaelT May 25 '13 at 15:50

This question has been asked before and already has an answer. If those answers do not fully address your question, please ask a new question.

8 Answers

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.

share|improve this answer
add comment

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.

share|improve this answer
add comment

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.

share|improve this answer
add comment

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.

share|improve this answer
add comment

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.

share|improve this answer
    
I seriously can't imagine how you can do Formal Code Review as described above in a consistent, scalable manner. You may be able to do it sporadically, when you feel like it, or you may be able to do it if your team is small and write very few code, but you definitely can't do it if you are under tight deadline or if your team is consisting of 5 developers and above. I think. –  Graviton Feb 18 '11 at 11:18
    
@Graviton, we were consistent (review every week, sometimes twice a week) but not scalable. I added some more details on this. I agree it can't realistically work in most real-life projects, that's why I suggest a "hybrid" alternative in my last paragraph. –  Péter Török Feb 18 '11 at 11:27
    
He did the review on his own, fixed trivial issues and sent the list of nontrivial issues back to the author to be fixed. I find this process a bit too monotonous and tedious; I afraid that developers will stop following it after some time ( when the pressure from managers ceases) –  Graviton Feb 18 '11 at 15:59
    
@Graviton, we had specific review tasks in the backlog to ensure that it doesn't get forgotten. As long as I was in the team, it worked well, but that was only 6 months. I have no detailed info on the afterlife of the project. –  Péter Török Feb 18 '11 at 16:33
add comment

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.

share|improve this answer
    
@Tim, may I know what source control you are using? Also, after the description about after the pull how the review takes place is a bit vague, mind to elaborate? –  Graviton Feb 18 '11 at 8:34
1  
@Graviton - Both Mercurial and Git. We build on top of a lot of open source code, so we basically stick with whatever upstream is using. I don't recommend heavy uses of branches in Mercurial (just go with individual repos), but with Git they are quite helpful. –  Tim Post Feb 18 '11 at 8:36
    
@Graviton - To clarify, I don't have anything against how Mercurial does branching, but it does confuse a lot of people. –  Tim Post Feb 18 '11 at 8:37
    
@Tim, some of the points which I would like to know is: 1. how the reviewer does the review? Does he go through line by line of the changes? How often he does the pull? If too often, he would be spending all the time reviewing, if too infrequent, the changes get piled up and nobody likes a marathon review session. Did he review for big architecture changes ( like "you should create base class and then subclass rather than copy and paste")? –  Graviton Feb 18 '11 at 8:41
    
@Graviton - A diff is sent via e-mail for each push to an individual repository, which everyone concerned reviews as they have time. If someone sees something that is questionable, they simply reply to the e-mail and raise a discussion. By the time we get to pull requests, the only issues that tend to come up is combining the work of several teams into the main repository. –  Tim Post Feb 18 '11 at 8:43
show 4 more comments
  • 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.

share|improve this answer
1  
I like the last point. If you don't allow developers to make decisions you will never attract good ones. –  CashCow Feb 18 '11 at 12:54
    
Regarding Qualified reviewer: a peer is generally qualified. Lots of good points in your post! –  Matthieu M. Feb 18 '11 at 18:39
add comment

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.

share|improve this answer
    
@jmpquigley, mind to share on how frequently you do your code review? As soon as the code is check in? Once a week? Do you allocate a fix time in every week to do this? –  Graviton Feb 18 '11 at 7:50
    
Our code had to be checked in, proven to compile and run (with full unit testing) before it was brought up for a code review. When the review would occurred was dependent on the schedule, but it was my responsibility as the lead developer to schedule it before it went to formal QA testing (it couldn't go until I did the review and it was approved). When I would create coding estimates for the work it had to be included in the estimates; it was part of the process. –  jmq Feb 18 '11 at 7:57
    
@jmquigley, do you do daily builds? In our setting we have daily builds and if I follow your strategy, I have to personally approve all the changes on every single day, probably before the build starts, that's a bit too taxing for me, I guess? –  Graviton Feb 18 '11 at 8:23
    
No, we do weekly builds, on Friday, with my current project. With my last project the main build was bi-weekly. It took ~24 hours to do the build, so they couldn't do them daily. We also had 3 different versions going at the same time. The project was >20 million lines of code and 40-45 sub projects. A daily code review would drive me nuts, lol. You do a daily merge into a main branch every day? –  jmq Feb 18 '11 at 8:36
    
@jmquigley, yup, daily build. I'm even thinking about doing continuous build in the future. But currently no need. –  Graviton Feb 18 '11 at 8:37
show 2 more comments

Not the answer you're looking for? Browse other questions tagged or ask your own question.