13

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?

Eugene
  • 1,943
  • 1
  • 21
  • 35
  • 1
    possible duplicate of [Should your best programmers have to check everyone else's code into source control?](http://programmers.stackexchange.com/questions/181817/should-your-best-programmers-have-to-check-everyone-elses-code-into-source-cont) – gnat Feb 08 '14 at 17:08
  • 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 09 '14 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 '14 at 09:38

5 Answers5

17

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.

jmoreno
  • 10,640
  • 1
  • 31
  • 48
  • 2
    "*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 09 '14 at 11:03
  • @JensG: but it's not a peer doing the review in the OP's situation. – jmoreno Feb 09 '14 at 18:35
  • 3
    That's why I made it bold. – JensG Feb 09 '14 at 22:22
8

In this situation, what you need is for the knowledge of this experienced developer to help the rest of the team grow. The quality of a team is not defined by the skills of the best developer; it's defined by the skills of the worst. You can try things like:

  • Collaborative reviews. This worked really great in my last team. We put the whole team in a room with a projector and started reviewing some items. Perhaps at the beginning the architect is the one that guides the review, but in a few weeks (we reserved one or two hours every Friday) the whole team starts to talk and understand the key concepts that currently only the architect seems to know.

  • Pair programming. For me this is the best tool for spreading knowledge in a team.

SH7890
  • 277
  • 2
  • 12
AlfredoCasado
  • 2,159
  • 11
  • 11
  • +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 09 '14 at 11:02
3

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.

Jacob Sparre Andersen
  • 314
  • 1
  • 5
3

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.

sea-rob
  • 6,841
  • 1
  • 24
  • 47
2

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
mihai
  • 307
  • 3
  • 11