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.

Does your team rely on code reviews for approval of work? Is it simply QA that signs off your features and fixes? Do you have a lead programmer (or architect) that reviews code design and implementation?

Or is it simply a Works on My Machine paradigm?

What would you recommend or have seen that works well?

share|improve this question

closed as too broad by Thomas Owens Sep 10 at 18:06

There are either too many possible answers, or good answers would be too long for this format. Please add details to narrow the answer set or to isolate an issue that can be answered in a few paragraphs.If this question can be reworded to fit the rules in the help center, please edit the question.

1  
I think you should try adding some context, about your own situation, to your question. What kind of industry are you in? Who are your customers? How big is your company? On what kind of platform are you writing on? On a side-note, have you read "Code Complete"? –  Stephan Branczyk Jan 6 '11 at 22:55
1  
    
@Gnat - I am really looking for a question that says basically how should a code review work? That should be on topic but I figure that would be a dupe of this question right now. –  Chad Sep 10 at 15:44
    
@Chad such a broad topic is unlikely to be adequately covered in Stack Exchange Q&A format. Consider taking a look at more concrete questions tagged code-reviews, in particular at How should code reviews be Carried Out? and those linked to it –  gnat Sep 10 at 16:02
1  
This question should have been closed. If you want to know how a code review process works, ask it. But first do a search. –  BЈовић Sep 10 at 16:02

5 Answers 5

Code reviews can be immensely helpful in identifying potential bugs and making sure that developers adhere to company coding standards. However, it's important to have clear guidelines about what is and is not up for review. A written coding standard—accessible via a web browser—is a must, in my opinion. And everyone on the team should be given a chance to provide input as to what goes in the standard. When a reviewer flags a defect, he should cite the section of the standard that the code violates, and provide a hyperlink to it.

Having a written standard and requiring that reviewers cite it for each item they mark as a defect has a couple of important effects:

  1. It keeps reviews from getting too personal. Reviewers are merely checking for compliance with an agreed-upon standard, not 'bashing' the developer whose code is under review.
  2. Developers can review code much more quickly when they have a 'checklist' of things to look out for.

I once worked for a company that decided to implement a work flow that used Gerrit for code reviews, and required a review before code could be committed to the dev branch—a decision I was strongly in favor of at the time. And I still am. Unfortunately, we didn't have a written coding standard. A handful of senior team members thought that this would be "overkill", or "too restrictive", and felt that a de facto standard existed within the company culture that "everybody already knows". Predictably, reviews devolved into an endless debate about whitespace and curly brace placement, and heated discussions regularly occurred on topics that had nothing to do, really, with code quality or executing the company's mission.

So that's the best advice I can give: have a written, web-accessible coding standard (like this one), and require that reviewers stick to it scrupulously when flagging defects. Many software developers tend to have OCD tendencies (I suspect that's what makes us good at coding), and if you don't tamp down on this in code reviews, they can become a bully pulpit for those that can't stand this:

if (!initialized_) {
    initObject(obj);
}

so they keep trying to sway people to do this instead:

if ( ! initialized_ )
{
    initObject( obj );
}

Distinctions like this almost entirely devoid of meaning, and are never a determining factor in the success or failure of a project/company. So either leave brace placement explicitly unspecified in the standard, or just pick something and require everybody to live with it. Better still, use something like uncrustify in a commit hook to enforce a 'standard' syntax. A different company I worked for did that, and I thought it was weird at the time, but I totally get it now. Anything is better than having developers waste money endlessly debating trivialities. We can do that over lunch. (And we're going to, anyway. It's like a sport for us.)

If you have a published standard that 'hits the high spots', I think the dev team itself can handle code reviews. That's how we do it in my current job. (We use Crucible, FWIW, and like it pretty well.) QA isn't involved in the review process at all. A developer who wants to merge his code simply creates a review and invites other developers to join. He has a green light to merge when everyone has finished the review and all defects have been addressed. There's no strict enforcement. You 'should' create a code review before merging your topic branch—and almost everyone does—but circumstances occasionally call for a 'rogue' merge. For our team of 25+ engineers, this has worked out pretty well.

share|improve this answer

Having QA review for code before it goes into production is definitely good practice and is recommended. General code review by other team developers is typically done before QA. However, the size of the team, the specific product, culture, etc. determine how the code review should be done. What I recommend is to do a code review with multiple people reviewing code. Also if you have several team members I would suggest having several people review the code not just the lead developer. Team members with less experience might not be able to identify as many issues, but will learn not only new ways to do things and become more familiar with the code that the team supports.

share|improve this answer

In our case, code ships when it meets the specified requirements. A thorough testing, review and validation process of the software is undertaken to insure that the requirements are fully satisfied.

Although we have informal style guidelines (and review of critical code), that is a secondary consideration. I work with a lot of very smart people, and code quality is seldom an issue.

share|improve this answer
    
Wow, that must be nice! But smart != mistake proof. –  morganpdx Jan 6 '11 at 23:34
    
That is pretty much exactly how we run things where I work. –  Pemdas Jan 6 '11 at 23:53

Code review is very effective when contineous and implicit within the team.

That's why it's not explicit in our Definition of Done.

Here is how it works:

  • Tasks are not assigned, team members can decide to either work alone on any task or partner with another developer.
  • They communicate trought the whole process and review each other.
  • Sometimes they decide to do pair programming which is, IMHO, the best code review process ever.
  • Pair programming ensure produced code is not owned by an individual and meet the team's coding guidelines.

With this system, it is very unlikely that a developer will work alone on his module without any external help. This behavior is natural when tasks are not assigned.

It's why it's very important to have your team located at the same place, in the same office or splitted into two 3 desks rooms.

share|improve this answer

Continuous education (by the architects to the developers) of the stuff and a team spirit where everybody is responsible for foreign work too. Control in sense of quality assurance is good but do not sufficient in critical project situation (dead lines). You need stuff that work hard to achieve the impossible, all together.

share|improve this answer