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.

In my company mostly the architect does code reviews. He is a very experienced and smart software guy, so he is very good at it. When developers do the code reviews they don't do it half as well. We tried giving developers to do more code reviews, but the quality of the code reviews wasn't good. We use Scrum for as development methodology.

However with the current system there are two problems:

  1. The architect becomes a bottleneck

  2. Developers don't take the responsibility for the quality of the code and the architecture (which leads to all sorts of problems).

How can we address these issues? Should we change who does the code reviews?

share|improve this question
1  
1  
I don't see it as a duplicate. The questions are related, but the possible duplicate focuses on slightly different issues. –  Bart van Ingen Schenau Feb 9 at 15:20
    
Could you expand on what you mean by the 'quality of code reviews'? Do you mean the quality of the code that emerges at the end of review? Sounds to me like you only have one developer that can produce code of acceptable quality, in which case I'd say you have bigger problems... –  AakashM Feb 21 at 9:38

5 Answers 5

Developers should do code reviews. They should do code reviews, because they should know the code, the company style standards and practices. By having someone else do code reviews, you are telling your developers that it's not their responsibility to make sure the code meets the companies standards.

If you think they need training on doing code reviews, get it for them. Given your current situation, you could have a dev do the code review, and then have that commented by your architect -- have the dev submit the review to the architect for approval before sending it on to the submitter.

share|improve this answer
1  
"By having someone else do code reviews, you are telling your developers that it's not their responsibility to make sure the code meets the companies standards." - yes and no. You are also telling "Your code is subject to (hopefully) critical peer review, so you better do it right first time." –  JensG Feb 9 at 11:03
    
@JensG: but it's not a peer doing the review in the OP's situation. –  jmoreno Feb 9 at 18:35
3  
That's why I made it bold. –  JensG Feb 9 at 22:22

In this situation what you need its that the knowledge of this experienced developer helps the rest of the team grows, the quality of a team its not defined by the skills of the best developer its defined by the skills of the worst. You can try things like:

  • Collaborative reviews, this works really great in my last team, put the whole team in a room with a projector and starts reviewing some items, perhaps at the beginning the architect its the one that guide the review but in a few week (we doing reserving one or two hour all fridays) all the team start to talk and understand the key concepts that now only the architect seems to know.

  • Pair programing, for me this is the best tool for spread knowledge in a team.

share|improve this answer
    
+1 for pair programming. In fact, my first though on that question was "everybody", but pair programming nails it better. I think we get the most out of it if we make it a source of learning besides the quality aspect. –  JensG Feb 9 at 11:02

While I can see the point in having the system/software architect sign off all changes/commits, the software developers should be able to do reviews without involving the architect - except for arbitration.

My preferred[*] review procedures are:

  • Group changes by requirement/issue.
  • Invite all developers, the software architect and the author of the requirement/issue to review. (They are not all required to do a review.)
  • Consider a review finished when:
    • Two developers have reviewed.
    • All comments are responded to. (Possibly by having the software architect make a decision.)
    • One working day has passed without further discussion (or all invited parties have reviewed).

So my short answer to your question is: The developers should do change reviews.

[*] Unfortunately this is not always how the projects I participate in operate.

share|improve this answer
    
now always, or not always? –  Martijn Feb 13 at 20:50
    
As you may have guessed: "not always". Thanks for spotting it. I've corrected the answer. –  Jacob Sparre Andersen Feb 21 at 8:04

I agree, if just one person does reviews, the rest of the guys will probably just go with "I don't know, it seems to work, but let that smart guy figure it out if it's ok or not". I can think of the following:

  • make your code public so that everyone can see what everyone is working on; put out names at the start of each file that contains code; maybe print them out and stamp them in the, uhh, bathroom or wherever you feel it will catch some eyes
  • pair programming; with another brain beside you, you'll think twice before naming your variable i
  • pick up your janitor and explain him how that inheritance works (oh, yeah, it doesn't work). Explaining your code to someone else helps. Maybe it compiles, maybe it does the right stuff, but you didn't really understand why. Now is your chance
  • have a set of guidelines and make everyone follow them; whatever the guideline, it's better than no guideline
share|improve this answer

I like the practice of occasional team code reviews that include the whole team the architects, but then lots and lots and lots of code reviews between two or three members of the team.

If it's really tricky or sensitive code, then enlist the architect or senior members of the team.

Honestly, though, it sounds kind of ridiculous having an architect do code reviews. He should be doing design reviews, or occasional code reviews informally to share his expertise. The engineering team should take responsibility for the code. If there are problems, then they will get better at it with time.

share|improve this answer

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.