28

I suspect major code review cover up in my team. Too many code reviews are merged without any comment.

Seems to me like there's no such thing as a code review without a single comment.

How can I as a team lead properly monitor that my team is doing a proper code review process and how can I help them to maximize the process' benefits?

Update

Thought people might want to know about any update. I tried a lot of suggestions that were given here. most were already in use. some helped a bit. However, the problem remained - some people continuously got bad code in when I was not looking.

I found that code review monitoring is not as helpful as giving my team tools to make their code better to begin with.

So I added a library named "jscpd" to detect copy pastes. The build failed on copy pastes. That eliminated one problem immediately.

next we are going to try codeclimate.

I am also doing a manual review on old code reviews once a sprint for half a day. I am converting todos into issues/tickets - as I found out people are writing them, but they are never handled at a later point. I am also doing meetings with the entire teams to review code when it is appropriate.

in general it feels like we are moving in the right direction.

Improve this question
8
  • 1
    In case you are using TFS, you can configure it to incorporate Code Reviewer's Name.
    – Krishnandu Sarkar
    Commented Mar 10, 2015 at 9:40
  • 1
    possible duplicate of How to deal with someone who dislikes the idea of code reviews?
    – gnat
    Commented Mar 10, 2015 at 9:40
  • 11
    @gnat I disagree. There's a difference between someone disliking code reviews and what this question is asking about. This question can be attacked from a traceability perspective (linking changes in source code to the review, or defects/enhancements/stories to reviews of that implementation, etc) or from a process quality and auditing perspective. Both have implications, even if people generally don't have a problem doing the code review.
    – Thomas Owens ♦
    Commented Mar 10, 2015 at 12:17
  • 3
    Do you attend any of these reviews? Maybe it's time to drop in on one? Point out a few things yourself & ask each reviewer individually why he missed all of them?
    – Mawg
    Commented Mar 10, 2015 at 12:28
  • 3
    Do you find that obvious problems have not been spotted by the review? Would you have added (important) comments?
    – usr
    Commented Mar 10, 2015 at 16:47

7 Answers 7

Reset to default
72

I'm going to offer a different take from my fellow answerers. They are right - be involved if you want to see how things go. If you want more tracability, there are tools for that.

But in my experience, I suspect that there's something else going on.

Have you considered that your team may feel that the process is broken/stupid/ineffective for most commits? Remember, process is documenting what works well, not rules to obey. And as the team lead, you're there to help them be their best, not enforce rules.

So in your retrospectives (if agile) or one on ones (if you're a manager) or in random impromptu hallway meetings (if you're a non-agile team lead and there's another manager doing one on ones), bring it up. Ask what people think of the code review process. How is it working? How is it not? Say you think it's maybe not benefiting the team as much as it could. Make sure you listen.

You can do some advocacy for code reviews in these meetings, but it's better to listen to the feedback. Most likely, you'll find that either you team thinks that the "proper" process needs adjusting, or that there is some root cause (time pressure, lack of reviewers, Bob just commits his code so why can't we) to address.

Forcing a tool on top of a broken process won't make the process any better.

Improve this answer
3
  • 5
    +1 for the right approach to this (and many others!) problem
    – Olivier Dulac
    Commented Mar 11, 2015 at 10:20
  • 7
    +1 for the last sentence. This is something that almost nobody understands, but is extremely important.
    – JohnEye
    Commented Mar 11, 2015 at 14:08
  • 1
    Nice answer. Tried that.. My team says "the company is doing stuff the wrong way. we need more qa.. and let developers develop" while the company says "we want the developers to submit good quality code. we aim to disperse the qa team since once the code is in good quality, qa are no longer needed.."... What happened eventually is that people who got bad code in continuously got fired and I reconstructed my team.
    – guy mograbi
    Commented Aug 6, 2015 at 6:48
43

I dislike posting one-line answers, but this one seems appropriate:

Participate in the process.

Improve this answer
2
  • 16
    I also dislike one line answers. Fortunately you took two lines - and my answer. +1
    – Mawg
    Commented Mar 10, 2015 at 12:29
  • 1
    I am. but when I am not.. stuff happen. that's exactly what made me suspicious in the first place. I started re-reviewing other's review, and found out nasty stuff.
    – guy mograbi
    Commented Aug 6, 2015 at 6:44
6

Get a tool, like ReviewBoard or Redmine's codereview plugin. Then each review is created as a task that has to be closed or commented upon by someone (just like a bug ticket). Then you have traceability of who created the review ticket, and who closed it. You can tie review tickets with source code checkins, ie create the ticket from a revision.

Improve this answer
2

A few things (to be honest, most of these are covered across the answers, but I wanted to put them in a single place)

  • You can put process and rules in place to make sure a code-review happens, but it's pretty impossible to put them in so that code-review is actually more than a box-ticking exercise. Ultimately the team has to see the benefit of the process, if they are to approach it usefully

  • Lead by example. Take part in reviews. As a developer, I feel bad if my manager (a non-developer now) spots stuff I don't. Highlight issues that should have been caught in review (in a non-blaming way). If a production issue happens, if issues arise during QA (if you have a separate QA process), highlight where they could have been caught in code-review. Discuss with the team how can we can ensure future issues like that are caught

  • Discuss with the team what they want the process to do. If they don't see any point to it (as may happen at the beginning) use the production issues and necessary refixes as evidence of its benefit

  • Use automated code-checking software like Sonarqube so that code-reviews can focus on issues like incomprehensible-code, logic errors, lack of documentation, etc that can't be spotted automatically.

Improve this answer
2

You could document what the team wants in code reviews that you've discussed and agreed with developers. Some things you could consider as part of code reviews are:

  • Check that the code does what it's supposed to do i.e. it meets the requirements

  • Code style to ensure that developers are coding to a consistent style

  • Optimisation e.g. number of function calls

  • Architecture and reusability

  • Exception handling and logging

  • Technical debt: is the code in a better state than when the developer started working on it

  • Check out and build the code (I find this useful but other devs in my team prefer to leave this to testers)

  • Using an automated tool (I've used SonarQube). I find it useful to integrate this into your build process to enforce improvements to the code e.g. increasing the test coverage

Some of the steps above can be covered by an automated tool but while you're trying to improve the way code reviews or done it's probably worth using both the tool and eyeball review. However, the most important steps for preventing technical debt (architecture and reusability) cannot be wholly automated.

If your team is inconsistent in applying this you could try only allowing the developers who are carrying out code reviews properly to have merge rights. For example, you might just want to start with the lead dev on the team. The trade-off with this approach is that those developers could become a bottleneck in the development process, so you and the team need to decide if you want this. Personally I would accept this trade-off and through the code reviews increase the discipline across the rest of the team and then when ready you can increase the number of developers with merge rights.

Finally, it's worth reviewing the reviews. So once a week get together with the developers and constructively discuss the reviews and ways of improving them.

Improve this answer
7
  • is this an advert for SonarQube? I tried it - I wouldn't recommend it, far too painful to get going and whilst "open source" costs for all the useful bits.
    – gbjbaanb
    Commented Mar 10, 2015 at 14:37
  • It's running fine in my current team and wasn't too difficult to setup and it's helping - it's not an advert but it's the only tool of this kind i've got experience of. Would you say the same for Redmine codereview and ReviewBoard?
    – br3w5
    Commented Mar 10, 2015 at 14:44
  • We're using SonarQube in our teams, serving around 70+ projects, ranging from 10k to 3M LOC. Although some teams just ignore its reports, most use it to direct refactoring processes. It works nice, although personally I prefer simple, non-integrated tools, such as Findbugs.
    – Dibbeke
    Commented Mar 10, 2015 at 21:03
  • And here was me thinking that code review involved checking if the code matched the design document :-/
    – Mawg
    Commented Mar 11, 2015 at 7:58
  • 1
    Thanks, this is what I am doing at the meantime. I will update within a couple of weeks how it affected.
    – guy mograbi
    Commented Mar 13, 2015 at 21:44
0

I'll tell you how my team quickly integrated code review into its workflow.

First, let me ask you a question. Are you using a version control system (e.g. Mercurial, Git)?

If your answer is yes, then proceed.

  1. prohibit everybody from pushing anything (even small fixes) directly to the master branch (trunk)*
  2. develop new features (or fixes) in separate branches
  3. when developers believe the branch is ready to be integrated in master, they will create a "pull request"
  4. prohibit everybody from merging their own pull request*
  5. have another developer evaluate the pull request and review new code
  6. if the code passes the review, good, the pull request can be merged, otherwise fixes can be applied
  7. repeat step 6 until code matures enough (can be done without starting over)**
  8. done, all your new code gets reviewed (at least summarily), by someone with a name

Now you have a precise point in your workflow where code review gets done.

Act there.

* can be enforced automatically, with server-side hooks

** this procedure is fully supported by GitHub (among others), and is fairly easy to use, check it out

Improve this answer
6
  • 2
    Even with such a process (which I supposed actually happens from the description in the question), you sometimes have developers thinking "ah, I trust my colleague enough and have too much to do myself, so I'll just merge it without actually reading the details, or even commenting on it". (We have a similar process in our team, with two approvals needed (from people other than the PR author), before it can be merged. Still sometimes changes go through without a thorough review.)
    – Paŭlo Ebermann
    Commented Mar 10, 2015 at 20:57
  • 1
    @PaŭloEbermann I see. I'm afraid that's an inevitable outcome of circumstances, if you don't have enough time to do everything you need to, quality will suffer, one way or another. Sill, if it does not work "sometimes", that means it works "most of the time", no?
    – Agostino
    Commented Mar 11, 2015 at 0:07
  • 1
    Yes, it helped a bit by allowing the merge only for a limited set of people, who had the task to check if the actual review was done right.
    – Paŭlo Ebermann
    Commented Mar 11, 2015 at 8:31
  • I had a similar prohibit, and needless to say: the development almost stopped. That rule lasted whole 2 weeks, after which the managers had to adjust their plans.
    – BЈовић
    Commented Mar 11, 2015 at 10:50
  • @BЈовић was your team doing code reviews on a regular basis before? This technique is used by many, especially in the Open Source ecosystem. The fact it didn't work for your team does not mean if can't work for others.
    – Agostino
    Commented Mar 11, 2015 at 14:54
-2

I think you should create a template and ask your team members to update it every time they do a code review. But even then, you should participate in the review process initially.

Improve this answer

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