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?
Software Engineering Stack Exchange is a question and answer site for professionals, academics, and students working within the systems development life cycle. It only takes a minute to sign up.Sign up to join this community
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.
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.
You may want to read through this free book:
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.
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.
Should you do formal code reviews?
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:
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 :)
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).
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.
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.
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.
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.