13

I'm a software developer in a 7-8 developers team. We've been doing code reviews for quite some time now and code quality has improved over time.

However I recently noticed that some developers are being asked for more code reviews than the others. I'm afraid that's because of their flexible attitude.

In my view, this is not how code reviews should be done: all the team should be accountable for it and code reviewers shouldn't be chosen for their willingness to easily accept changes.

How do you deal with this issue in your team?

Have you established rules for choosing code reviewers?

Do you think code reviewers should be rewarded for the time they spend doing (good) code reviews? And how should they be rewarded?

Thanks for your answers/ideas.

Improve this question
4
  • 7
    Have you considered creating a round robin system where both the coder is left in the dark about who is reviewing and the reviewer is left in the dark about who is the coder?
    – Neil
    Jul 13, 2018 at 10:27
  • 1
    I haven't, but I like this idea! Thanks!
    – guillaumegallois
    Jul 13, 2018 at 10:35
  • 1
    Who is in charge and why don't they do their job which should involve making sure everyone else does theirs?
    – JeffO
    Jul 14, 2018 at 23:42
  • On my team, reviewers are automatically assigned whenever a pull request is opened. Reviewers are selected from the team round-robin. We have a webhook for each of our repos that assigns reviewers automatically whenever the PR is opened. It basically keeps a list of all the devs, and who was last assigned, and just works its way through the list.
    – Dan Jones
    Jul 17, 2018 at 19:48

5 Answers 5

Reset to default
12

We don't choose reviewers.

In our team:

  • All code changes must be code reviewed before the Pull Request is completed
  • At least one developer must code review (two or more reviewers are better and having testers, analysts and other team members doing code review is also extremely beneficial)

Code Reviews aren't assigned, people pick them up when it works for them. The understanding is that we can't push stories through the pipeline. On occasion someone will mention that they're awaiting a CR in the standup but that's about it.

I like this model, it gives people to pick up what they can and avoids "giving people jobs".

Improve this answer
6

Introduce a rule that a bug may be assigned for fixing, not only to those who committed the change, but only to those who reviewed it. That should create correct perspective for the code review.

As for,

Do you think code reviewers should be rewarded for the time they spend doing (good) code reviews?

Well I'm not sure how generally developers are rewarded for doing their job except just getting a salary and being kind of proud of what they have done. But as reviewing code is part of their job, the reviewer should get time for the review, and share the credit somehow.

Improve this answer
5
  • 2
    It's an interesting idea, but a lot of times when a bug comes up, 90% of the work is figuring exactly what's causing the bug, and 10% of the work is fixing it. Doing a post-mortem to figure out exactly what change introduced the bug may not even happen, unless it aids figuring out what's going on, or how to do a safe fix.
    – DaveG
    Jul 13, 2018 at 11:02
  • You made a good point about the credit that code reviewers should be given. This is definitely a problem that should be tackled. Thanks for your answer!
    – guillaumegallois
    Jul 13, 2018 at 12:26
  • I think it might get people to try to not do code reviews at all or might be that you will not have any work done because people will start nitpicking.
    – Mateusz
    Jul 13, 2018 at 13:00
  • 5
    This answer assumes the goal for code reviews is to find bugs. That's not the case, the primary goal is to keep the code in a maintainable and evolvable state (and if you are lucky, some bugs are found as well).
    – Doc Brown
    Jul 13, 2018 at 14:00
  • @DocBrown to prevent bugs and also to make sure future fix of the bug will be faster (both by familiarizing the other developer with the code and by making sure the code is well written)
    – max630
    Jul 13, 2018 at 14:13
4

The main problem you have is not technical, but some tools can lower the amount of effort that code reviews take. There are a few challenges to overcome:

  • Understanding what the change is. Git Pull Requests on feature branches really help this process along.
  • Making the code review an event where people have to be in the same room. This adds the stress of scheduling, meeting resources, etc. GitHub, GitLab, and BitBucket allow asynchronous reviews so they can happen when the peer is ready.
  • The ability to provide meaningful feedback when looking at code. To be honest, the line-by-line commenting feature in GitHub, GitLab, Bitbucket pull requests really are more useful than a face to face meeting. It feels less political.

That isn't to say that you can't use SubVersion or other tools (like Fisheye) to help, but the integration built into the Git pipeline with feature branches really does make the job less of a chore.

Outside of tooling, you need to look at more social challenges:

  • Have your developers start their day by reviewing any open pull requests to sign off on it.
  • Have your developers review any open pull requests before they start a new task
  • Require approval from more than one person for your pull requests.

It might also be worth checking what types of tasks are being code reviewed by the more engaged people. They might be grabbing all the easy reviews, which makes things more painful for everyone else.

Improve this answer
2
  • The last point is good. I once worked with a developer in a small team who would only ever review changes to software he had written which caused significant bottlenecks elsewhere in the team.
    – Robbie Dee
    Jul 13, 2018 at 13:02
  • In that case, it sounds like you have someone trying to protect their "territory". As much as possible, you want to foster a sense of community ownership for the code. In other words, there aren't protected sanctums within the code that no other developers except the "blessed" can touch. I understand if there is a specialty gap and you can't review the math, but you can at least review how well the code matches its intent.
    – Berin Loritsch
    Jul 13, 2018 at 14:35
2

A good idea is to do it on a round robin basis - you pick someone who has done the least number of reviews for your code. Over time, everyone in the team should have done roughly an equal number of reviews. It spreads the knowledge too.

Obviously there will be occasional exceptions like holidays where there will be peaks and troughs.

There should be no distinction between juniors and seniors/leads. Code reviews should be carried out for everyone's code - no matter how senior they are. It reduces friction and helps to share different approaches.

If you are still concerned about the quality of the reviews after all this, consider introducing a set of minimum standards for a code review to pass. What you include is entirely up to you but some things you might want to consider are are code coverage, unit tests, removing commented out code, metrics, sufficient comments, build quality, SOLID principles, DRY, KISS etc.

As for incentivising code reviews, quality code is the reward. We've all I'm sure worked on sub-optimal code bases where the pain could have been lessened considerably had another developer just given the code a once over from the outset and suggested constructive changes.

Improve this answer
0

It sounds like the team lacks a formal process for code reviews.

I'm not talking about a creating a 350 page Word document, but just some simple bullet points on what the process entails.

The important bits:

  1. Define a core set of reviewers. No general statements. Name people.

    These should be your senior developers.

  2. Require more than 1 core reviewer to sign off on the review.

  3. Identify at least 1 other non core reviewer each sprint or release who is a temporary core reviewer. Require their sign off on all code reviews during this time.

Item #3 allows the other devs to rotate in to the core reviewer group. Some weeks they will spend more time on reviews than others. It is a balancing act.

As for rewarding people? Many times acknowledging the effort a person is making during code review in front of the whole team can work, but don't stress yourself out over this.

When in doubt, define the process and tell the team they need to stick to it.

And there is one last thing you can try — controversial as it may be: let the @#$% hit the fan, if I may use an idiom.

Let the team fail, because the code review process is not being followed. Management will get involved, and then people will change. This is really only a good idea in the most extreme cases where you already defined a process and the team refused to abide by it. If you don't have the authority to fire people or discipline them (as most lead developers don't) then you need to get someone involved who can do this stuff.

And there's nothing like failure to get things to change. Despite what people might say, you can steer the Titanic — but not before it hits the ice burg.

Sometimes you just need to let the Titanic hit the ice burg.

Improve this answer

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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