How to be better at reviewing code?

Question

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?

Answer 1

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.

Answer 2

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

Answer 3

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.

Answer 4

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.

Answer 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.