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.

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?

share|improve this question
9  
This sounds like informal / casual code reviews, and code review is a Good Thing (tm). We even have a sister site for code reviews. –  Yannis Rizos Jul 13 '12 at 9:52
1  
+1 code reviews as a practice tool –  Yusubov Jul 13 '12 at 9:55
2  
Supports a learning environment. It's as much for the viewers as it is the writers. –  MathAttack Jul 13 '12 at 11:56
3  
As long as it stays collaborative, its good practice. There are some company cultures (up or out) where colleagues are internal competitors. In these circumstances code reviews require social/political skills in addition to technical skills. In that case I'd say it's too stressful. The best code reviews are the informal ones between colleagues: "hey I just pulled an update and saw the code you checked in yesterday. Maybe it would be a better idea if you ... rather than ...". Collaborative and beneficial, not competitive. The projector idea somehow feels like a "let's throw tomato's" excursion. –  Louis Somers Jul 13 '12 at 23:10
2  
One of the main benefits of code reviews is that you automatically write better code if you know it will be aired in public. –  James Anderson Jul 19 '12 at 1:48
show 3 more comments

12 Answers

up vote 50 down vote accepted

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
share|improve this answer
1  
Yes, code reviews are good. But won't it slow down developers ? –  Radu Murzea Jul 13 '12 at 11:37
4  
@SoboLAN - Well, if code reviews mean that bugs are caught earlier and bad designs fixed before they have a chance to go into production, what do you think? –  Oded Jul 13 '12 at 11:50
9  
@SoboLAN: quality, speed, price - pick any two. –  Den Jul 13 '12 at 12:30
6  
It is also the best way to improve and keep consistency of code base, i.e. ensuring that team members understand and share the coding idioms preferred in the project, and use them correctly. –  Péter Török Jul 13 '12 at 13:09
3  
@SoboLAN: In my experience code reviews speed up developers. You catch more bugs, faster, and get to harmonize your solutions with what other developers are doing. –  Tynam Jul 13 '12 at 13:52
show 2 more comments

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)

share|improve this answer
1  
Your 2nd bullet could be broadened. 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. –  Dan Neely Jul 13 '12 at 13:44
    
@DanNeely good comment, i will include it for sure. –  Yusubov Jul 13 '12 at 13:46
add comment

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.

share|improve this answer
    
I absolutely agree with you, but I hope that our team is professional enough not get embarrassed but learn instead. –  superM Jul 13 '12 at 16:42
2  
Finally a reasonable answer. I found it far more effective to do revievs with just the developer and a single reviewer than doing it in a group. This way it's much easier to deal with mistakes for both sides and you can occassionally slide into pair-programming without wasting the time of the others in the group. –  x4u Jul 13 '12 at 17:04
4  
@superM, the rule is "praise in public, criticize in private" for a reason. –  HLGEM Jul 13 '12 at 17:12
    
+1 for timing. Best is to code in pairs, test-first. But in any case you want to review code while you are still willing to rewrite it. There's little point in reviewing code if you aren't willing to do major cleanup. I have been in code reviews where the original developer had taken 50+ lines of code to reimplement a library function. But since that code had been tested, replacing the 50 unnecessary lines with a single function call was not allowed! This turns a 10,000 line program that can be maintained by half a developer into a 100,000 line program that requires four. –  kevin cline Jul 16 '12 at 4:19
add comment

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.

share|improve this answer
add comment

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.

share|improve this answer
add comment

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.

share|improve this answer
add comment

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.

share|improve this answer
    
+1 we can all 'make it work' but it's more about making sure the code is readable and maintainable, particularly by following conventions. Not the most exciting work but very valuable. –  Kirk Broadhurst Jul 16 '12 at 1:58
add comment

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!

share|improve this answer
    
I've been working for only/already 1.5 year, and I want those code reviews for similar reasons. )) –  superM Jul 13 '12 at 18:34
add comment

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

share|improve this answer
    
'Chasing the rabbit' sounds very familiar to me )). but still I hope we'll manage to introduce code reviewing, especially when our managers don't mind at all. –  superM Jul 13 '12 at 17:00
add comment

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.

share|improve this answer
add comment

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.

share|improve this answer
add comment

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.

share|improve this answer
1  
without an explanation, this answer may become useless in case if someone else posts an opposite opinion. For example, if someone posts a claim like 'Code review obscures things, making it harder to see "the whole"', how would this answer help reader to pick of two opposing opinions? Consider editing it into a better shape, to meet How to Answer guidelines –  gnat Nov 13 '13 at 16:37
add comment

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.