15

We use Scrum with short sprints of 1 or 2 weeks and use peer reviews to improve product quality. How do we best use code reviews as quality assurance tools?

  • pre-commit reviews - hold tasks until the reviewer can finish them. Reviewer are on the critical path, and experience more pressure, the tasks take longer to complete. Developers may be idle developers if tasks are dependent.

  • Post commit reviews risk code reviews dropping to low priority. Feedback comes late (so comments and suggestions can be outdated) and in general it does not add much value since the code may be already released.

Which is the best "flow" for peer reviews? What is your experience?

Improve this question
6
  • 3
    For me this question is not PM related. The Scrum Team decides when to do it to maximize the work flow. Some teams do it before PO review and at the same time they write acceptance tests. It depends on what are the workflow elements and how you work with the code repository. But as I said... I believe it's off-topic. – Bartosz Rakowski May 27 '11 at 20:11
  • 1
    I agree that this is not a PM question. This question would be better asked on Programmers SE since it's a programming question. As a PM, I am not officially involved in code reviews. I do look at code every now and then, but not as a PM, but as someone with a software engineering background. – jmort253 May 29 '11 at 21:52
  • 2
    I believe this is a relevant question. As a PM you have responsibility for the success of your project. Part of that will involve making sure code reviews at completed, although you should be led by the developers that does not mean you shouldn't be able to suggest options they may not have thought about. You should be able to lead the team to the right decision and background info will only help the discussion. – dlongman May 25 '12 at 8:37
  • 2
    Voting to close, as this question is not framed from a project management perspective, and "What is the best...?" questions are inherently polling questions. – Todd A. Jacobs♦ Jan 6 '13 at 19:43
  • I went ahead and closed this as off-topic. Since the question was narrowed to where the asker was asking the best of 3 possible options, not an unending list, I wasn't sure not constructive was appropriate. However, the overall response from the comments resulting from Bartosz's post is that Scrum from a development perspective, not PM, is not on topic here. If anyone disagrees or would like to discuss this further, please create a Project Management Meta discussion or use your reopen votes as necessary. – jmort253 Jan 7 '13 at 1:08
7

With Distributed Version Control, you don't have to choose

Even if the answer is to "let the Scrum Team decide", that doesn't mean the question is not PM-related, or that no good answers can be provided here. It's perfectly within the purview of a good PM to provide their team with work-flow options that they've found have worked well in the past, or that other teams like. That's the spirit in which this question should be answered.

I think it's a mistake to conflate when to do a code review with when to commit. This sounds like thinking mired in a single-repository, Subversion-type mindset. If your team is using a distributed version control system (DVCS) like Git or Mercurial then these are really two separate issues, or at the very least the trade-off the questioner is worried about is not longer an issue.

In a DVCS, each developer has their own repository, and they should commit there as often as they have a changeset for which they'd like to store meta-data, or be able to revert to, etc., and this is independent of any code review.

The Scrum Team, or any small group of people working on different parts of a single solution, has a team repository. Each developer is responsible for pulling and merging the latest-and-greatest from the team repository into their own before pushing any of their new changes back. Then, when the team decides their set of changes is ready for review, they should push their changes to a code review repository. That way they can continue working (if necessary) in the team repository while code review happens on the baseline in the review repository.

Once the code review is complete, the changes (plus any revisions) are pushed to a stable repository, which is used to construct released builds.

This process enables commits to happen frequently. Commits should absolutely happen prior to the code review, as that's what sets the baseline of things to be reviewed. The beauty of DVCS is that this commit doesn't impact the version of the code from which released builds are made.

This process also solves the problem deprioritizing code reviews, because although the developers have pushed their code to the review team/process it is not yet released. So developers can continue working, but the code doesn't ship until the review is done and the code gets pushed to the stable release library. This should keep the pressure on the review process while not preventing the team from continuing to work.

This process is essentially what's described at the end of the 5th chapter of Joel Spolsky's overview of Mercurial.

Improve this answer
1
  • 1
    While I don't disagree that answers here can be useful, this isn't the site for any useful material, it's the site for project managers to ask questions related to project management. Some may argue this is a PM question while some may not. I consider this to be a boundary question as it's bordering being off-topic. With that said, great answer. Distributed version control means commits can be made frequently, which is what version control systems were designed for. – jmort253 May 29 '11 at 21:57
6

I encourage teams to put a WIP (work in progress) limit on the code review column, usually 3.

When the column is full, developers can no longer put something into that column. Instead, they have to either review some code or chase someone else to do a review. This is a Kanban practice rather than Scrum, but I usually find it's the first useful place to put the WIP limit on the board (the "ready for test" column is usually 2nd).

Putting a WIP limit on the board allows senior developers to review code in their own time if they have it, while ensuring that nobody spends too long without getting feedback on their code.

Improve this answer
2
  • "while ensuring that nobody spends too long without getting feedback on their code." This is key and raises the issue of cycle time and sizing. We have had work go off into the woods for over a week before getting reviewed because it was simply too large in the first place. – worldofchris Oct 30 '12 at 12:41
  • Good point. I tend to get devs to split off the thing that they find riskiest - usually something they've never done before - and try to find a way to focus on that, first. That may mean that they do the GUI with hard-coded data, for instance, with persistence of that data as a story to follow. Slicing vertically a la Agile is excellent but not the only way to cut it. – Lunivore Oct 30 '12 at 18:02
2

From developer perspective:

Commit change(s) to private branch, then send them to review. While waiting for review, work in an another private branch on an other issue. When review is complete, fix issues and then push your changes to main branch.

Improve this answer
3
  • The branch strategy is a great idea and works really well with Git. I find "send them to review" troubling, though - it doesn't actually prompt the reviews to get done, so the branches can continue to pile up. Also find when the reviews are being done that it's better to sit as a pair and fix the issues as they're found - much faster learning! – Lunivore Oct 21 '12 at 10:59
  • The branch strategy is working very well with any SCM which supports proper branching and merging, even with Subversion (with help of additional tools, like Savana). – Volodymyr M. Lisivka Dec 10 '12 at 13:48
  • @Volodymyr-m-liviska If you haven't tried Git, you won't be aware of how much better it is than Subversion. Git is fast. Very, very fast. Having said that, Subversion is a pretty good start, Git wouldn't exist without it, and I didn't know about Savana which probably helps a lot. – Lunivore Dec 11 '12 at 9:28
2

There was an article and discussion here, Software Quality Connection - Does Pair Programming Obviate the Need for Code Review, where I took exception to some assumptions in the math of the original (which I maintain was far wrong), but in which we discuss code review v. pairing v. both.

Improve this answer
1

We also do 1 week sprints and require code review. Something about your post caught me as strange. You mentioned that code review was required, yet you mentioned if you do the review post-commit the code may already be released. I think that may be the key, it sounds like you need another branch in there somewhere. Our developers commit their code to a common development branch, then perform code review on that branch. Once a user story is accepted, all the code for that story can then be merged into trunk so that it can be released. So the review happens post-commit in the development branch, but pre-commit to the trunk.

Improve this answer
1

Which is the best "flow" for peer reviews?

it depends. If the purpose of code review is to guard your codebase against someone checking in junk, then you should have mandatory reviews before check-ins; there could be reasons like

  • you do NOT trust your developers (outsourced project to some remote team which you don't know and don't trust at all, or, you just hired a junior developer and you want to make sure his/her code is good enough)
  • the pressure is too high (you have to fix a few bugs before the major release -> you don't want to introduce new bugs)

The reasons to do code review AFTER check-ins could be:

  • you want all your team members to understand what is going on
  • you want to make sure everyone is following Clean Code principles
  • you want to brainstorm existing code/architecture and improve it etc

in any case code reviews should NOT turn into ego contest, humiliating comments should be strictly forbidden, everything must be done in a friendly, professional way.

Improve this answer
0

From the PM perspective I have only one answer: let the Scrum Team decide.

Improve this answer
5
  • 1
    Thus the Scrum Team has to make an objective and reasoned decision, but your answer wouldn't help them. – Christian Strempfer May 31 '11 at 8:54
  • I would be more wordy if the question was asked on Stack Overflow, but I deterred myself from divagation. – Bartosz Rakowski May 31 '11 at 9:30
  • I believe we should answer the questions from the PM perspective. The team can have problems with almost anything: selecting framework, working hours or catering menu - it does not mean I should answer such questions here, on PMSE, just because it would help them. @Sis asked the question about Scrum and for Scrum there is short answer, which, I believe I provided. An unkind reader may call it whatever she likes. Not that I don't care, I just think we should keep PMSE as "clean" and PM related as possible. – Bartosz Rakowski May 31 '11 at 12:19
  • 2
    You've provided a consultant's answer. Technically correct, but not very helpful. OP asked for experience and advise. While I recognize the value in delegating to and trust the Scrum Team, I also recognize that as the PM, I'm accountable for advise/assistance/facilitation/whatever to successfully close the project. – Mark C. Wallace Jan 2 '13 at 11:33
  • Hi Bartosz, I was trying to think of a way to edit your post to add in your explanation from your comment here, but I'm not sure I can do it without significantly changing your post and putting words in your mouth. I'm thinking a good way to address the commenters here would be to explain in your answer that, as far as a PM is concerned, he/she shouldn't be involved in the Scrum process, and it might be worth explaining why, in your experience, the PM shouldn't be involved. Hope this helps! :) – jmort253 Jan 4 '13 at 4:03
0

I feel, the code review should be done before committing the code, you could ensure it by including it in scrum as well and allotting some minutes for code review.

Improve this answer
1
  • I disagree. Code should be committed whenever it compiles. Version control is there to help make sure code is always backed up. Most version control software has tools to rollback to previous versions of files if necessary. – jmort253 May 29 '11 at 21:54

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