-1

for clarity, code review = a team meeting and reviewing/sharing code

What are the potentially negative aspects of the code review process in a development environment? What comes to mind for me

  • Strong criticism can lead to negative interactions and feelings of animosity between colleagues.
  • Laziness to read and understand new code can lead to apathetic and very general feedback.
  • Considerable Time investment is required to organize and execute the review process.

There must be more than this though, What negative things have you guys noticed or experienced?

closed as primarily opinion-based by gnat, user40980, GlenH7, mattnz, Bart van Ingen Schenau Dec 2 '13 at 8:59

Many good questions generate some degree of opinion based on expert experience, but answers to this question will tend to be almost entirely based on opinions, rather than facts, references, or specific expertise. If this question can be reworded to fit the rules in the help center, please edit the question.

  • 1
    None of the items in the list are of any concern for a properly designed code review procedure. – mattnz Dec 2 '13 at 3:49
7

This might be mostly just my opinion, but I'm basing this opinion on what my teams have tried in the past and what we've observed.

Our management has tried to institute/enforce team-based code reviews and I've always hated them. In fact I've hated 90% of what management tried to institute/enforce because it seems like most those things did nothing but waste time.

The problem with team based code reviews is that you get a whole bunch of people in a room. All these people should have looked at the code ahead of time (very expensive for all team members to stop their work and look at yours) but they never do (because everyone has their own work). So meeting starts and presenter starts talking at people and pointing at code they've never seen. This meeting will only last 1-2 hours, so in the end only maybe 10-20% of code will get "looked" at. And because no one had the time to really think and understand what they are looking at, the only thing they can comment on is style or nit pick on tiny details which are mostly irrelevant.... the whole thing was a complete waste of time.

At the same time I love code reviews and I make sure that every single line of my code gets looked at. What worked for us in my previous company when management stopped trying to push their "process" and worked is continuing to work for us in my current company is "asynchronous" code reviews:

  1. I work on a feature and write a bunch of code
  2. When code is ready, I submit it for code review. I pick a reviewer but generally I just go to the team and ask who would like to do the honors (one person most of the time)
  3. Reviewer gets a notification e-mail
  4. I switch branches (if using git) or shelve the code changes (if using perforce) and go on to other work
  5. When reviewer is done, I get an e-mail back with his feedback.
  6. I switch branches (or shelves) and modify my code based on his feedback and push/commit it.

Each code change has different importance. If it's trivial/easy/simple, we don't even talk about it face-to-face. Everything is done with tools/emails. If it is more important, after reviewer had a chance in peace and quite on his own time to sit down and understand what I wrote, we hold a meeting and go over his feedback. If it is even more important, we have multiple people in that meeting. But the key is that we always have a single "primary reviewer". This way no one goes into the meeting thinking that other people have looked at the code. If you are the primary, then you know you HAVE to look at it.

Another good thing about this model is that most of the time interaction is one-on-one between reviewer and reviewee, so it doesn't feel like an interrogation where you stand in front of the group and everyone critiques you. Also we keep our reviews extremely informal. There's no hierarchy. Every developer can ask any other developer to send a review and in the end our goals are:

  • Get a second set of eyes on the code because there's always something silly that will get missed
  • Get someone else to be familiar with the code and with your style
  • Reviewer can learn from you if you did something they've never seen before
  • You can learn from the reviewer if they did something you've never seen before and they noticed you are solving the same problem the hard way

I found this method to work for us and waste minimal time as the whole team doesn't get put on hold and you do get valuable feedback/interaction because reviewer is actually engaged and has a chance to really sit down and take as much time as needed with no time pressure and on his own schedule.

  • Thanks for your response, it's really helpful to get other more experienced perspectives. – Eogcloud Dec 1 '13 at 18:19
0

Strong criticism can lead to negative interactions and feelings of animosity between colleagues.

It's most likely going to be this. Some people don't like to hear that what they did was wrong, or rather, not quite right. The team as a whole has to agree on what is acceptable, but there will always be those that still do not agree with the rest of the team and so may grow to loathe the other team members. This is human nature I'm afraid.

  • There is a lot of literature on the subject that discusses how to avoid this problem. Personally I have never experienced it. – mattnz Dec 2 '13 at 3:55

Not the answer you're looking for? Browse other questions tagged or ask your own question.