Code Reviews, what are the advantages?

Question

In my team, we don't do formal code reviews. We tend to think that it's enough with pair programming and rotating pairs often.

Should we consider doing formal code reviews? What would be the advantages?

Related question: http://programmers.stackexchange.com/questions/15874/tips-on-persuading-boss-that-code-review-is-a-good-thing You may find some of the responses there interesting. — Kevin D, Apr 27 '11 at 09:15
People are missing the point about code reviews... Not only do they check code for correctness, but they spread blame around if something later turns out to be wrong. With pair programming it's you or the other guy who gets the chop. — James, Feb 18 '13 at 02:33

score 9 · Accepted Answer · edited Apr 27 '11 at 13:22

9

We do code reviews a bit different (maybe).

We come up all programmers together (every Friday) and look what we have done in a weeks period. Then we chose what projects we want to review so that every done/in progress project would have at least one or few people. Then in hour or so we look at the changes that was made, search for mistakes, how other projects work and etc. Afterwards we discuss, tell the mistakes, how it should be done (we don't fix bugs we just point out them and spam the code with FIXME). All in all it usually for us (10 programmers) takes around 2 hours.

The pros:

Every member knows what happens in the company
Bugs are found faster
It forces us to write readable code (code that can be read without explanation or introducing!)
Optimization methods/tricks/productive programs spread faster
Programmer as a specialist "evolve" faster
It's fun

What I have against pair programming as mentioned (sure it's only my personal opinion) is that the longer the team works together - the faster it gets.

I hope it would bring some food for thought. Good luck.

edited Apr 27 '11 at 13:22

Michael K

15,539
9
61
93

answered Apr 27 '11 at 06:35

JackLeo

1,977
12
20

What are you referring to when you say "the longer the team works together - the faster it gets"? And how is that a bad thing? – Edgar Gonzalez Apr 27 '11 at 18:10
Team gets to know each other there fore they can finish tasks faster. You don't get that if you constantly mix pairs. – JackLeo Apr 27 '11 at 18:14
Oh, but you get to know the whole team better, and you also get better general knowledge of the problem domain, as well as 3 or 4 different opinions from all the team members and not just from one :) – Edgar Gonzalez Apr 27 '11 at 23:45
You get the same during code reviews. :) more over you get the opinion on every case from every company programmer. Just have to wait for few days. – JackLeo Apr 28 '11 at 08:08

score 4 · Answer 2 · answered Apr 27 '11 at 08:45

You may want to read through this free book:

http://smartbear.com/best-kept-secrets-of-peer-code-review/

Sure, they have a product to push, but there's still a lot of useful information in there.

They also discuss how pair programming provides some of the same advantages, so if you're pair programming you might not need code review at all.

score 2 · Answer 3 · answered Apr 27 '11 at 06:13

I don't have a lot of experience in reviewing in your environment. We don't do a lot of pair programming here we do code reviews to spread knowledge of the software in the team, have another pair of eyes to pick out the mistakes and have a formal point to check if the software sticks to our coding guidelines.

The first 2 points are fairly good covered by the pair programming, the third is very dependent on the pair and could get better from a formal code review.

score 2 · Answer 4 · answered Apr 27 '11 at 06:27

2

Should you do formal code reviews?

Absolutely

Just as a quick side note, I have very little experience with paired programming, but I don't believe that reviews would conflict with these methods.

I'd introduce two forms of code reviews:

Peer code reviews

Even if paired programming works for you, it never hurts to get another set of eyes on the code. The benefits to this are:

It gets a set of fresh eyes on the code, someone who may have more intimate knowledge of areas of the codebase that you (and/or your partner) may not be as familiar with. This can expose knock-on issues.
It makes you (the pair) re-validate your solution prior to submission. As the reviewer knows nothing about what you've written, you have to explain it in its entirety. This can expose edge cases that you hadn't thought of, or invalid logic.

Peer code reviews (in my world) are conducted prior to every submission. How this carries over in the paired programming world, I'm unsure of.

Group code reviews

These happen less frequently than peer code reviews. I would generally pull my group (or a subsection of my group) in a meeting room for an informal code review. I'd generally pick some code that was written by a random person on the team, preferrably code that was written from scratch - refactored code doesn't expose issues like fresh code.

Make sure everyone knows that these reviews are not meant to emberass and are not used to reflect performance. They are merely to ensure that your team coding standards are followed and to help everyone be better engineers and thus, become more useful to the team (and further career growth, etc etc) - and make sure that this is the true intent of the reviews. If anyone suspects anything different, these will become feared and less productive.

I would go through the code somewhat informally, letting anyone in the room point out different solutions that they may have or logic flaws that they encounter. This is meant to be more of a group discussion than a leader sitting there telling everyone how they should code.

I've found that employing these two methods increases the rate at which engineers progress as well as lowers bug counts considerably :)

answered Apr 27 '11 at 06:27

Demian Brecht

17,555
1
47
81

2

"It never hurts". Well, yes it can. Code reviews are expensive and can be a huge waste of time, which would be much better spent deliver working software. – Martin Wickman Apr 27 '11 at 08:11
@Martin on the flipside, peer review increases your truck number. Having the only guy who knows what X does die is a huge waste of time as you try spin up a replacement. – Frank Shearar Apr 27 '11 at 08:41
@Frank Yes, but we are comparing formal reviews with pair programming and pp works justa good (better imo) with keeping the truck number manageable. – Martin Wickman Apr 27 '11 at 09:30
@Martin: If you honestly believe that, then I'm willing to bet that you've been on the end of ineffective code reviews. Generally speaking, I've heard that code reviews are a "huge waste of time" from the same people who insist that technical designs are not a requirement for development. After *years* of developing in a *high* pressure environment, I can tell you unequivocally that code reviews *are not* a waste of time. Code quality goes up, bug counts go down, knowledge transfer/sharing goes up. – Demian Brecht Apr 30 '11 at 02:38
@Demian, again we are comparing with pp which is code review, but it happens constantly. That makes it better than formal code reviews in my experience. Peer review is essential, but there are several ways to do them. – Martin Wickman Apr 30 '11 at 08:20
@Martin: While pp is apparently effective for some people in certain domains, *both* people are intimately familiar with the code. The strength of peer reviews over pp is that the person reviewing the code is completely detached from the work done. This gets a fresh set of eyes on the code written, along with explanations that must be given by the author (which alone can yield errors). – Demian Brecht Apr 30 '11 at 17:36
@Demian. True, but pp and rotating pairs gives you the benefit of having everyone on the team working on all parts of the code. That avoids silos (ie truck number issue) and brings shared understanding of the system and its design. I think formal reviews is something you must do if you are not using pair programming. If you do use pp, then reviews are optional and probably wont give you much benefit, as least not compared to spending that time producing software instead. – Martin Wickman Apr 30 '11 at 20:28
@Martin: Guess we'll just have to agree to disagree.. I'm far from an advocate of pp.. Might be that I just don't know enough about it (doesn't work in my line of work :)) – Demian Brecht May 01 '11 at 00:09

score 1 · Answer 5 · answered Apr 27 '11 at 07:43

I have never done pair programming in practice (only hoped for it), so I can't directly compare the two practices. However, I can tell my experiences with formal code reviews.

I used to lead formal code reviews in an earlier project, on legacy code. The project was in a complete mess and management welcomed any initiative with a hope of bringing order into chaos. At that time I thought formal code review was a good idea. We did find bugs, and we did see that the quality of freshly written code was significantly better than that of old code. I collected statistics, bug counts etc. to prove this.

We did one session per week on average, involving 3-5 persons. Each session took about 3-4 hours of time per person (including preparation), and reviewed 200-300 lines of code (LOC)*. In this pace, during a period of over 6 months, we managed to review about 5K LOC, out of about 50K.

In retrospect, I feel that it was very costly. With this pace, it would have taken us 5 years to review the entire legacy codebase. OTOH having more than one session a week would have taken resources away from development. Of course, that is the typical dilemma with legacy code. But even reviewing all freshly written code formally would take a lot of time, slowing development significantly.

My conclusion is that formal code reviews are best done on newly written code, focused on the most critical parts of the code. The rest is better handled in a more informal manner, possibly via pair programming. This is just my current opinion though, which may change. I don't claim to be a code review guru or anything.

*This is the normal pace of formal code reviews.

Typical code review rates are about 150 lines of code per hour. Inspecting and reviewing more than a few hundred lines of code per hour for critical software (such as safety critical embedded software) may be too fast to find errors.

Quoted from Wikipedia (emphasis by me).

Martin Wickman · Answer 6 · 2011-04-27T11:59:28.580

The underlying reason code reviews exists is because isolated programmers need to meet and discuss their code and check that it conforms to their standard.

You don't mention any quality problems, so it seems your team is already doing enough code reviews through their pair programming. Awesome!

Correctly done pair programming makes formal code reviews superfluous. But try it for a few weeks and see how it works out, but I suspect you won't notice any improvements.

Keep in mind that code reviews are a tiring, expensive process and not something to be taken lightly. It essentially introduces a handover in your project which is costly and slows everything down. It is much better to make sure the code is correct in the first place, rather than trying to find problems later.

score 0 · Answer 7 · answered Feb 18 '13 at 02:05

For me the main advantage of code reviews is that it makes people write better code.

Knowing that your code will be read and reviewed makes you more conscious of readability and the "correct"ness of your code. When you know the code is going straight into the repository and no one else will read it unless they are defect fixing you tend to let things slip like not re-factoring field names when their usage changes, leaving unused methods hanging around in case they might be factored back in etc. etc.

score 0 · Answer 8 · answered Apr 27 '11 at 07:32

0

Maybe. Code reviews take time. They are only worthwhile if the time taken by the review is saved at another point in the process. What savings do you expect from code reviews? Are you experiencing difficulties that could be prevented by code reviews?

answered Apr 27 '11 at 07:32

kevin cline

33,608
3
71
142

score 0 · Answer 9 · answered Apr 27 '11 at 13:34

If you are doing pair programming, the need for a code review substantially decreases but you would certainly benefit from a peer review. For this to be beneficial it must be done by someone senior and more experienced person than the pair members.

What are the benefits? Well, it would be better if you consider the risks of not doing it.

The pair could be doing something wrong or maybe doing it in a sub-optimal way
Maybe you are not following coding standards or not documenting the code properly. A peer review is really good at finding these
No one other than the pair understands a particular piece of code. So what happens if both the pair members have left and the code is poorly documented? Others will waste time trying to figure things out. Having a third person who knows what you have done reduces the risk.

I am amused that people have said that code review is a waste of time. Yes , it does take time. Maybe it will not produce any change in the code but that doesnt mean it is wasted. Thats like saying you dont need to check your fire system regularly because it is a waste of time.

Code Reviews, what are the advantages?

9 Answers9

Absolutely

Linked