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?
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:
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.
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.
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.
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:
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.
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.
