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.

First I firmly believe in the code review process and always want someone else to review my code. My question really centers around how can I do a better job at performing a code review for someone else?

I know that to perform a code review you need to have knowledge of the how the existing code works and a knowledge of what the local standard is, both of which I feel that I know very well. Still I feel like I never do a good enough code review for other people. Also I know that certain people seem to do a better job review code than others so I am wondering for those that are great code reviewers what are the techniques that you use?

share|improve this question
3  
Could you go into why you feel like you don't do a good enough job? By what metric? –  Mark Canlas Jun 16 '11 at 2:36
    
    
Agree with @Mark: code review for correctness, style, simplicity, efficiency, ...? Are you able to spot bugs by reading the code? Are you able to spot inconsistencies in style by reading it? and so on. –  rwong Jun 16 '11 at 7:04

5 Answers 5

[H]ow can I do a better job at performing a code review for someone else?

Ask them lots of questions

I know that to perform a code review you need to have knowledge of the how the existing code works...

Actually, no, you don't have to know the code beforehand to be a good reviewer.

A couple of jobs ago, my employer started requiring all code check-ins to be signed off by a reviewer. I was doing mostly GUI work in C, and one of the best reviewers for me was my buddy Bill. He was proficient in C, but had never done much GUI work, and going into the reviews he had no idea how my code was supposed to work.

But he asked lots of questions about it, and having to explain so he could understand what my code did and why stimulated a lot of thinking on my part. It led me to find lots of weird little bugs with edge cases, and also consider other approaches I might have taken. Also, although I'd been writing C for 22 years at that point and thought I was pretty proficient, it quickly improved my code quality.

Even though I no longer work there, I still review diffs prior to check-in and ask myself, "What questions would Bill have about this?" And quite often, I wind up changing something as a result.

share|improve this answer

In my experience the best way is to let the hole team to make the code review. We use a commit mailinglist in each project where you can follow every code changes to the version control system. Most of our developer have subscribed to their project specific mailinglist because they are interested at code changes.

When someone notice a bad way in the new source code he either explains the committer how he can do it a better way, if the committer is a trainee, or he begins a discussion about it, if it was a more experienced committer.

Of course this method does not guarantee that all new code is reviewed, especially in stressful times when no one of the team member has leisure to follow each code change. Also not every developer is responsibly to ensure that every developer makes his job good, alone of this you can not guarantee that it is reviewed. But, at least in our teams, there is always a technical manager who is responsible for technical quality.

I am a real fan of code reviews if they conform to following scores:

  • every developer has the possibility to review all code and argument to his opinion
  • no one has the right to abuse others code
  • not only bad code activate a discussion but good code too
  • the discussions ends with happinesses for every involved people
  • the review occurs nearly in real-time, at least before feature is completed

What I have learned is that if you are someone who review each line of code and think you have to control such things like code quality in terms of code formatting or code efficiency then you self are very inefficiency because you do things what machines can do for you. Your target should be to use a continuous integration system which controls the build and code quality of each code contribution. If this system generates reports and sends them to the contributors everything is perfect.

I must admit that if you have to review the code because you have to control, or to rank the quality of a programmer, then my suggestions does not make sense. In this case I would also not review the source code line by line. I would review things like:

  • are there security relevant issues
  • are intended APIs used
  • did the code apply the specified architecture
  • did he write useful tests (but only if he was instructed implicit, I had to learn)
  • Documentation
  • build-process
  • ... and some more, probably

If you are an experienced developer you will definite always find things like loops which you could do with better performance. Of course it is useful to explain others such knowledge but this should be not part of the review session. If there are significant performance issues then not because he (or she) used a less efficient variant of a list type.

Because the initial question was why some people seems to make a better review as other people I would answer that these people perhaps make a preview before the real review begins, means they probably are prepared them self so that they know exactly what they want to review.

share|improve this answer

There is no way to do better code review. Only thing you can do it keep that improving with learning and experience.

Normally things I follow

- Use variables judiciously
- Keep things in scope loose boundaries will generate more errors
- Orient your language of coding in domain specific terms, they make more sense
- Keep loops to minimum 2 for each method if needed
- use ternary operators
- Arrange methods alphabetically
- Keep errors at handling ease
- write less but efficient code

I think there lot you can add to it.

share|improve this answer
1  
I am not sure arranging methods alphabetically is that good idea. I'd say keeping them ordered by their function would be better. Having two related methods really far away just because they are named getSomething and setSomething doesn't seem that good of an idea. –  devoured elysium Jul 9 '11 at 23:05
1  
TBH, ternary operators a lot of times turn your code into something harder to understand than without them (although more verbose). –  devoured elysium Jul 9 '11 at 23:09
1  
I am also not too sure what you mean about "write less but efficient code". I'd say generally it shouldn't matter how much you code as long as it is clear -- I don't particullary care for efficient code most of the time. –  devoured elysium Jul 9 '11 at 23:10

ask yourself what makes others a good reviewer for you?

also, as you go through the code;

  • stop at anything you don't understand now write that a comment is needed
  • identify if it conforms to the coding standards: spaces, brackets, camelCase..etc
  • check that it includes all functionality
  • do simple testing of logic to see if it passes boundary conditions etc..
share|improve this answer
1  
reason for downvote? constructive criticism please –  Ross Jun 16 '11 at 5:14
2  
Capitalize properly. –  Mark Canlas Jun 27 '11 at 20:25
1  
lol what? np bro –  Ross Jul 5 '11 at 4:43

I just aim for

  • explaining why a suggested change is needed. Making sure I get the reason across not just the fix
  • agreeing on code formatting - so that everyone's code looks the same/familiar
  • sharing a list of code-traits that you want to maintain. Put it up on a wiki so that everyone doesn't have to make every mistake once. Update it frequently.

Apart from that, "knowing what to look for" just comes with experience, practice and reading up.

share|improve this answer
1  
I'm a big fan of mechanical code formatting. Ideally done via a preprocessor during checkins, so people can avoid the official standard if it really bugs them (experience suggests that they quickly give up) –  Мסž Jun 16 '11 at 3:51

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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