Saturday, August 1, 2015

Agile Peer Review

Preface

What follows is primarily the result of nearly 2 years of intentional refinement by an experienced, high-performing agile team. Dev team members, architects, product owner, and even members of other dev teams participated and helped shape our thoughts on review. I also incorporated ideas from several other experienced peers. But like anything agile, it's a work in progress.

Don't let preconceptions about the term "review" color your reading - most of the reviews called for are only 5-15 minutes long.

Overview

Functional defects and design problems get exponentially more expensive the longer they are allowed to exist. The earlier those problems can be found and fixed, the better. Peer reviews provide an inexpensive, effective method for doing that.

There are several major work products in the lifecycle of a user story that warrant review (these are discussed in detail below):
  • Domain Model - The team’s shared, fundamental, documented understanding of the domain.
  • Demo script - How the dev team will demonstrate to stakeholders that the story is functionally complete.
  • Design - Focused on the API and its test cases, but also covering important implementation details. 
  • Pull Request - A single coherent change to the shared codebase, in service of a story. 
  • Code Complete - The full set of changes to the codebase that were made to complete the story.

Benefits

These reviews deliver tangible value to the business

  • Higher-quality code 
  • Higher, more consistent velocity

And additional value to the dev team

  • Pride in work 
  • Self-improvement

As a result of the direct effects of the reviews

  • Getting a better product, sooner 
    • Bugs identified, eliminated earlier in the process 
    • More maintainable code
    • Better tested
    • More adherent to team/organizational standards
  • Knowledge sharing
    • Team members sharing the same frame of mind
    • Better understanding of the domain
    • Better understanding of how system components interact
    • Improving our craftsmanship every day by learning from each others strengths
    • Better understanding of our tools and technical stack

What to Review

Domain Model

The fundamental, documented understanding of the domain shared among the whole team including the Product Owner. A critical part of this model is the Ubiquitous Language, which unambiguously expresses domain concepts, enabling effective communication. Any change in that understanding or expression needs to be reviewed by the rest of the team, to keep everyone on the same page, and to consider potential impact to the software design. Not every story changes the Domain Model.

Demo Script

How the dev team will demonstrate to stakeholders that the story is functionally complete. This would involve the dev team and Product Owner. Depending on team/org structure, more or less of this might be provided to the dev team by PO, or might even be developed collaboratively, in which case a separate review would be unnecessary. 

There is not a lot of detail here, just a handful of user-facing scenarios. If it is more than a handful, the story is probably too big.. It shouldn't take much time at all to produce or review. If it does take a long time, again it's probably too big, or there is a lack of understanding that needs to be addressed before proceeding. For example:
  • Create an event
    • Go to the Event list page
    • Click "New Event" button
    • Fill in information in Event form, click save
    • Should show Event overview page with saved info
    • Go back to Event list page, new event should be there
  • Register for an Event
    • Registrant Role
      • Go to Event Registration page Fill in form, submit
    • Admin Role
      • Go to Event overview page, see new Registration

Design

For a microservice, this would focus on the API and its test cases, but would also cover important implementation details. Generally should be developed collaboratively with the major client(s) of the API, e.g. UI. In which case, the review with the rest of the team shouldn't take long.
  • Resource endpoints
  • Methods on those endpoints
  • API objects
  • API test cases
  • API versioning
  • New runtime dependencies, e.g. now we're going to have to use the Org service
  • Major changes to infrastructure, e.g. using a new database, new message queue
  • Data migration

Pair Programming

This is effectively continuous code review between two team members while the code is being written. I wrote up most of my thoughts on this practice here. Because of the normalizing and quality control forces exerted here, the pull request and code complete reviews go faster.

Pull Request

Code gets merged into the shared repository by issueing a pull request. This should occur frequently, typically multiple times per day per developer. It should be coherent and correct, but not necessarily complete. Each pull request gets reviewed by one or more other team members. In my experience, the types of things caught here are team standards violations, code/test readability (e.g. method and variable names), and code factoring.

Code Complete

When the dev(s) working on a story consider it code complete, the code and its tests should be reviewed by the rest of the dev team, typically plus an architect.
  • The person(s) who did the coding walk everyone else through it
    • Generally start with brief review of the story
    • Then walk through the important parts of the code
      • And the tests!
  • While those persons are presenting, someone else is taking action item notes
    • Somewhere public, or just email out after
  • The story is not considered dev-complete until those action items are addressed.
    • Creating a tech debt story to do it later is a valid option but should not be abused 
  • This is at a level similar to Design review, focusing on API classes and tests, and any deviations from the original design.

How to Review

When to review?

Each work product should be reviewed as early as feasible, because course correction is cheaper the earlier you do it. Also, the less time elapses between creation and review, the more of the context the creator(s) will still have loaded. Finally, by spreading the reviews out over the lifetime of the story (as opposed to reviewing everything at the end), you avoid the tendency to get mentally overwhelmed by the volume of work to be reviewed and just rubber-stamp it. 

On my team, we had one-week sprints, Monday to Friday. We took Monday morning to commit to a set of stories for the sprint, produce and review demo plans for all the committed stories, and produce and review design and test plan for the first one or two stories we were starting work on. All other reviews were performed as needed at the end of each day.

Why not just create together instead of reviewing later?

There is a tradeoff to be made between direct collaboration and ex post facto review. In my experience, you typically want to keep creative activities for a given story limited to 2-3 people, 4 at the most. Otherwise the cost/benefit starts to deteriorate.

How much time to invest?

For a team that has been together a while, the amount of time to target for each review is ~30 minutes for code complete review, and 5-15 minutes for the others. Expect more review time early on in a team's formation. However, team norming will drive the time down significantly, and performing these reviews accelerates team norming. You don't keep finding the same things in review, because you incorporate the feedback and get better.

Who does the reviewing?

The composition of the reviewing group varies based on what is being reviewed, and larger review groups will tend to use more person-hours, because:
  • There are more people spending the time
  • More people talk more
  • The members of large groups will be less familiar with what is being reviewed

Putting it all Together: A Sprint in the Life of a Story

  • Monday morning - Dev team meets to commit to stories for sprint, and devise demo script for those stories. Right after that meeting, they meet with Product Owner to review the demo script, adjust as necessary.
  • Monday afternoon - One pair of developers start on a medium-sized story. They come up with a design together. During the design, they realize that their Domain Model and Ubiquitous Language will need to change. Previously, Constituents could only register for Events. Now a Constituent can be designated as the "Event Organizer". They ask the rest of the team and an architect for a design review, scheduled for 30 minutes later. In the mean time, they check the proposed new terminology with the Product Owner, and add the term to the project's wiki. Then they go ahead and start implementing the design.
    In the design review, an overlooked test case is identified and added, and the format of a timestamp field is changed to suit the UI. Minimal rework in the codebase is required. They also take that opportunity to inform the rest of the dev team and the architect of the new Ubiquitous Language term. Monday end of day - The pair issues a pull request. One other developer is still in the office, spends 5 minutes reviewing it. Catches a few issues, which he comments on, but nothing serious, so he merges the pull request.
  • Tuesday, Wednesday - Several more pull requests issued. Since the rest of the team is there, they both review both pull requests. The architect notices the pull requests going by and adds a few comments as well. Late Wednesday afternoon, the pair working on the story determines that it is code complete, and schedules a team code review for end of day.
  • Wednesday end of day - Dev team + Architect code review for the story. One of the dev pair refreshes everyone's memory about the story, then walks through the code and tests. The other member of the dev pair is taking notes. A few minor issues are caught, and one potentially serious scalability issue. The team decides to consult historical metrics to see if it will be a problem short- or long-term. Once they determine that it would take about a year of usage at current rates to become a problem, they decide to add monitoring for this story, and add a tech debt story to address the scaling longer-term.
  • Thursday morning - Dev pair demos the story to the Product Owner according to the demo plan.

Further Reading

2 comments:

  1. This is really awesome! It's an amazing level of detail and clarity and very clear what you're optimizing for. Based on my own code review process, I made a list of questions for you to think about:

    What questions are you trying to answer in each stage of the code review process?
    In what situation types (patterns) do you find yourselves doing code reviews?
    What problem type (patterns) do you find yourself detecting? Are the problems situation-specific?
    How much time do you spend on rework immediately after a code review?
    How does the time delay between code being written and the time being reviewed affect the time spent on rework?
    How are you capturing lessons learned so they can be shared with others?
    How do you know if your knowledge sharing system (lessons learned) is effective? Can you test it?

    I would really love to see the Idea Flow data for the effects of implementing this on a team. If you'd be up for it, I'd love to do a case study with you. Here's what I'd like to do:
    As you teach the team, blog about challenges and discoveries with teaching the review process.
    Then find another team, teach them IFLF, teach them the review process and see if we could chart the team's improvement with IF data. That would be the coolest case study ever.
    I've already got a minor level of permission to gather IF data with my devs that are there, I used to come in every week to work with them. If you wanted to do it, I'm guessing they'd let me help.

    Really awesome stuff, Ryan! Would love for you to join me @AustinAPI to discuss "An Open Standard for Learnable APIs" https://www.linkedin.com/pulse/open-standard-learnable-apis-janelle-klein

    ReplyDelete
  2. Good stuff Ryan, nice day by day walk-through.

    One thing I wanted to emphasize is that the cadence and specific activities and timings you described were conceived by the team itself after trying different approaches and cadences, driven by the team's desire to achieve good balance between planning and execution, and not due to any external factors.

    Based on what I observed, I felt that the ratio of planning/design activities to execution activities might be on the high side for a one week cadence. I would encourage you to try lighter planning upfront.

    ReplyDelete