19

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?

Improve this question
1
  • 10
    Here, apparently, we don't have time for code reviews, but lots of time to deal with the resulting screw-ups. I wish I was joking.
    – MetalMikester
    Jan 19, 2011 at 13:10

5 Answers 5

Reset to default
12

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.

Improve this answer
2
  • LOL, good understanding of unit tests is a must. And the good news is that testing is just common sense - takes less time to figure out than say ... the time it takes to pick up a new language.
    – Job
    Jan 19, 2011 at 15:05
  • I find myself nitpicking at stuff when there is a lack of unit test coverage. When I see unit tests in a code review, that's the first place I look. If I see the unit tests are hitting the business requirements and sensible edge cases (check for nulls where appropriate, boundary testing on ranges of values) then I tend to not nit pick --- which doesn't mean you should pick at small things. It's just that the "proof is in the pudding." It's hard to argue with well constructed unit tests that are passing.
    – Greg Burghardt
    Apr 1, 2019 at 15:56
6

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.
Improve this answer
4

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.

Improve this answer
2

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.

Improve this answer
1
  • I always suggest to let it be up to the dev what he does with the review feedback. The reviewer doesn't necessarily know best and when agreement is mandatory you may need to invest quite a bit of time to educate the reviewer. I'd consider a final 'integration' check by a senior/lead developer though.
    – Joppe
    Jan 21, 2011 at 19:08
0

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
Improve this answer

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.