Is code reviewing good practice?

Question

When the company I work in hired new managers, they offered us to overview someone's code on every meeting. We have meetings every two weeks, so each time one of developers was to show his/her code on the projector, and others were going to discuss it.

I thought this would be great: each developer would be more careful when writing code and we could share our experience better. But somehow we forgot about this and the offer just remained an offer.

What are the benefits of this and are there any drawbacks?

Answer 1

Code reviews are a great practice.

It is probably the best way to learn from mistakes and to see how certain problems are solved by others. It is also one of the best way to maintain quality in a code base.

Code reviews happen in many companies, though it is difficult to say that there is a specific process that they all follow.

In more formal code review, a senior (or several seniors) will sit together with a developer to review their code, offering suggestions and teaching at the same time.

Additional benefits to code reviews (as commented on for this question), include:

• A great way to teach and learn
• They are one of the best ways to improve and keep the consistency of a code base (style and idioms)
• They help ensure all team members understand the style and idioms used in the project and how to use them
• Code reviews will speed up development as they catch bugs and design flaws early (so, though they may slow down initial development, they pay dividends in later development cycles)
• There is tooling support that helps streamline the code review process

Answer 2

Code reviews are very useful tool for learning, especially when you have new team member on-board. Well, it is also known as peer review process :)

There are different types of code reviews:

• Over-the-shoulder – where one developer looks over the code author's shoulder as the latter walks through the code.
• Email pass-around – Source code management system emails code to reviewers automatically after checkin is made. Extract from comment: Having failed at convincing management to allocate time for any sort of formal code review I've taken to looking over my coworkers changes whenever I pull new revisions down from source control using the history/diff tools built into Tortise
• Pair Programming – Two authors develop code together at the same workstation, such is common in Extreme Programming.
• Tool-assisted code review – Authors and reviewers use specialized tools designed for peer code review.

There is only one associated disadvantage where formal code reviews have require a considerable investment in preparation for the review event and execution time.

More references are listed below (for more deep dive readings)

• MSDN - Code Review
• Find and Fix Vulnerabilities Before Your Application Ships
• Wiki link

Answer 3

That particular practice seems inefficient and likely to be embarrassing - who wants their mistakes pointed up to a whole group of people. So if they can't choose what is to be reviewed and the code is not yet in production, this is likely to make people uncomfortable.

Depending on when the code is reviewed can make a big difference in whether the code review comments make it into the code or not. If the dev can choose and choses only production code, review comments are unlikely to get implemented. It's nice to have meetings where developers can show off some nifty technique they learned that other people would be interested in, but that's not code review. That's training.

We do code review of every piece of code before it is moved to QA. Every piece. It involves generally only the code reviewer and the developer. It doesn't go to QA until the code reviewer formally passes it. So the developer has to make the changes. We have found and quickly fixed many problems that QA might not have found (they find things we don't see in code review too). Further, it way reduces cowboy coding and quickly identifies those people who are not performing well so we can fix their issues and train or get rid of them before they do damage to our application. Code review time is part of our time estimate when planning the work so it doesn't impact the deadline at all. And actually, it saves time in the long run because the earlier a problem is found the easier it is to fix. Incidentally code reviewers are peers not managers and we involve people at all levels in reviewing code.

I personally have taught less experienced developers many better techniques through code review and I have learned some better techniques myself by reviewing other people's code as well as their comments on my code. Further code review makes sure that someone else understands the code which goes a long way toward making it more maintainable. Sometimes, the code works but the questions of the review make it plain that there will be mantenance problems because the code is hard to understand. Better to refactor in those cases while it is all still fresh in your mind than a year later when even the code author is left scratching his head and wondering why the code does such and such.

Answer 4

The problem with this type of code review, one developer every two weeks, the progress will be slow. It could be months before everybody has had a turn under the spotlight.

While code reviews can be good, there must be a reason behind them, and behind the procedure for doing them.

Several issue would have to be decided:

• Who would pick the developer, and how much notice would they be given. No notice reviews are traps.
• Who would pick the portion of code being reviewed: management, the senior developer on the project, or the developer being reviewed.
• Is the purpose to teach the person whose code is on the projector how to do something better, or is the purpose for the person whose code is on the projector to teach everybody else around the room how to improve their code.
• What percentage of code will be reviewed this way, is this going to be a part of the QA process?

If the people who proposed this plan don't already have answers to these questions it is possible that they read an article about how all great companies do code reviews, so we should do them also, without understanding the purpose behind them.

Answer 5

Code review is great practice, only when idea for code review comes from development team. Developers don't need projectors and presentations to review each other code. If they want to - they will prefer to go to conference.

When idea of code review comes from management - it can sound like investigation of low software quality, and can demoralize development team. I don't think that management team should be involved in code review process. Code review side by side with management team - very bad, killing and destructive practice.

Answer 6

Code review is an excellent practice, particularly if it is done by developers to share knowledge, and the ground rules are set ahead of time that suggestions and criticisms are meant to be CONSTRUCTIVE, and not to use an individual developer for target practice.

Managers who are not developers will be greeted with suspicion by developers if they decide to do code reviews. Most manager types will not want to get into the detail that developers inherently get into when looking at code. Most managers will also not understand why the developers critique one approach over another.

If you are wanting to showcase the good work developers are doing to management, then "code review" has a different meaning, and should not be as detailed as code reviews that are done to instruct/improve code quality among the developers. This kind of presentation could be helpful in demonstrating what developers do if the presentation can be higher level, and less code-specific, focusing on what the managers understand (value, ROI, etc). It might make managers understand that Joe has added significant value to the company by building X, which we can show saves Y amount of time, or Z dollars per order, etc. I think it might be worth the effort in showing the value of individual members of your team. Remember, however, that you need to be careful you don't overwhelm your audience with jargon, or too many levels of detail.

Answer 7

While I fully agree that code reviews are very constructive for teaching I would like to highlight, for me anyway, it is to ensure that the team's design patterns are being followed correctly.

We write a little prototype work and we refactor bits of code and while we feel comfortable with the product at the end the readability has been damaged - people can not look at it and see as clearly what is going on.

Independent eyes are always beneficial I find as you get stuck into certain modes of thought and this is at all levels of experience. You invested hours into the design and code and so code reviews are difficult to deal with when there is the fear your effort is going to be thrown out. Yet at the end you, hopefully, end up with a cleaner, leaner and more manageable application and the experience is ingrained in you.

For our team we do as @ElYusubov mentioned and use a tool specifically for Code Review - Crucible. People review, comment and sign off on code. Every week we sit down and review face to face interesting and/or complex pieces of code.

Answer 8

As a software engineering intern, I've found code reviews immensely helpful.

On my team, a code review is required for each commit (big changes end up being formal, smaller changes end up being 'quick looks'). I definitely feel as though my engineering/design chops have been boosted by this, especially since I'm more likely to pull out the whiteboard than the terminal right away since I know I'm being 'watched.' :)

In effect, I think it produces much better code with the only slight drawback being a slightly slower development time (which in my opinion, is worth it in the long haul when you don't have to fix/extend terribly designed code). Also, I think that if you have juniors/interns on the team they will appreciate the chance for valuable feedback. I know I do!

Answer 9

From my experience Code Review is really great thing if you perform it well. When you have professional / mature team members and managers. We as programmers are "solution solvers". Our job is not to create lines of "text". That's why we have to share ideas, mistakes, problems, experiences. Code review is really a good practice to achieve this.

Unfortunately it sounds great but it is really hard to implement in most of the companies. Your team needs a lot of "autonomy". Convincing your managers or financial department that not creating new feature is profitable seems hard.

In my company we are trying to do some code review. But too much time is spent with 'chasing the rabbit' ( making features ).

Answer 10

Much of style and basic syntax type checks are caught using tools these days (FXCop etc).

However, code reviews are good, especially with new members of a team, complex or high impact stuff (e.g. something that will be noticeable to important people if it fails or causes business impact) and especially when outsourcing or using short term contractors (especially again when they are non native speakers as translation errors/language problems can allow software to pass all the tests but not actually do what it is supposed to).

I am not a fan of putting code on a projector for a team to pick through - much better to have a code review meeting where another team member (team lead etc) goes through a listing with the dev. This impacts less people - stops a lot of wasted time on style arguments - and is less embarrassing for the dev. It is more constructive and easier for the dev to absorb real issues and not be blinded by "I would have done this..." sort of comments.

I also think that un-enforced code reviews - like putting code on a share or emailing it around in the hope someone gives up their lunch time to go through it - is a waste of time.

A sit down with a pile of listing, a marker and a cup of coffee in the coffee area is great for this.

Answer 11

This sort of group show and tell would be good for new technologies or to get several jr. devs up to speed, but I don't think it is as good as a consistent review of new code. It is more efficient to have to do it one on one.

Answer 12

Code review helps see "the whole". Separate developers have a tendency to see just their requirements/problems. CR discovers ideas and solutions from the whole perspective. CR mainly helps eliminate waste of unnecessary work. The whole product is cheaper and in better quality.