What does a standard code review contain?

Question

In my company, it's an email discussion of what feature is implemented and what kind of bug is fixed sent by the one who write the code. And the reviewer, who receives the mail, will review the code and discuss the quality and how to edit the code in his opinion. What does a standard code review contain?

Answer 1

In my experience, most formal code reviews devolve into style checking because it's easy. Even when you supply a checklist of things to look at, it's pretty easy for eyes to start glazing over.

I've found that unit test review provides more benefit. Most developers I've worked with don't really know how to unit test properly, and once they get that "Aha!" moment the rest of their code starts improving as well. Here's a hint: it's not a unit test if you require the user to inspect something, and it's not a unit test if you are just starting something up to run in a debugger.

Answer 2

It tends to vary based on what the issue is. A lot of times it's a simple rubber-stamp. "Here's what the problem was, look at this line here, it's obvious what's going wrong, and here's where I fixed it." "Yup, that's pretty obvious. Go ahead and check it in."

But when something more involved is going on, it usually goes like this:

• Run Check For Modifications on TortoiseSVN and get a list of changed files.
• Bring reviewer into your office.
• Explain the problem, with the CR from the bug tracking system open for reference.
• Go down the list of files in TortoiseSVN, opening each of them in BeyondCompare to view the changes.
• If the reviewer doesn't understand the changes, explain what you did and why.
• The reviewer may notice something that doesn't look good. If so, discuss it until you reach agreement on whether or not it should be changed. (If simple changes need to be made, you can even edit the file inside of BeyondCompare.)
• If you have made any changes, recompile and make sure it builds!
• Run the program to demonstrate to the reviewer that your fix actually works.
• Check it in.

Answer 3

IMO, A code review has nothing to do with features or bugs, but focusses on the quality of the code and tests written for it.

So, you sit next to your peer and have him explain the code, or take the code and go through it, whatever the situation calls for.

It does help when everybody programmes against the same standards and if you employ tools like fxCop to automate part of the process.

Answer 4

I prefer the code review where the dev sits with the reviewer and goes through the code line by line explaining it. Often, the dev will see a problem in doing the explanation that the reviewer may not have seen yet which is why this is my preference. I also do code reviews where I am sent the code and read it on my own and make comments, but I find those tend to take longer (I review and draft comments and send to dev who reads them and goes WTF does she mean and emails me back and I explain and two or three rounds later we get together and I point out on the screen what I mean and the dev goes, "oh yeah now I see it." ) and be less productive as there is less genuine discussion and more, "You did this wrong."

It is also critical to enforce standards in a code review but not to make them the only focus.

However, the code is not sent on to production until the code reviewer is happy or the manager (not the dev) has overruled him or her (code reviewers have been wrong too). This is critical or code review is just a bureaucratic process with no added value unless the code reviewer must approve the final code before it is pushed.

Answer 5

First you need to have coding standards and these are more than mere syntax. When people start in your company they must learn the guidelines of your company as much as possible before they start coding. If in the review process all kind of violations are found they will most likely:

• not be fixed due to time constraints
• found to be more annoying than what the guidelines are worth

The guidelines should make sense and there should be proper tooling to find violations and to refactor as easy as possible. Always look at the goal of the guidelines and the code review

The goal in my mind is to make the code as uniform as possible and to find issues with maintainability and readability. A secondary goal can be to get more people up to speed with a certain piece of software.

The guidelines in my mind could e.g. exist of:

• general syntax and coding guidelines (pick one that already exists and use tooling that checks automatically)
• Proper exception handling
• Proper logging
• Good use of the paradigms for the language (SOLID for OO languages)
• Obvious and well thought out dependencies between components (use tools like NDepend)
• Working build script
• Documentation present (developer startup, installation manual)
• internal libraries to use
• company policies
• third party tooling that isn't allowed
• Unit tests present and non failing
• code-coverage of 90%
• ...

With that in place the code review consist of the software being checked against the guidelines and:

• discuss violations with the programmer
• fix unnecessary violations
• comment necessary violations