Peer Reviews and Feedback

Peer review is an essential component of a functional team producing quality software. It allows knowledge transfer, stops obvious and sometimes not-so-obvious bugs reaching production, and it aligns everyone to process and coding standards.

In some companies, “peer review” is manager driven. The technical lead or architect reviews the code, and no-one else can. They become a bottleneck. Don’t do this.

In some companies peer review is handled asynchronously, for example by pull requests. This is great for smaller teams or time-zone distributed teams, and for code that the whole team should review (new framework added, security fixes, for example) but the cost is that it increases the cycle time by introducing a delay (which could be minutes or could be days) between code being submitted, reviewed, reworked and accepted. Unless they do optimistic merging, which I’ve already dismissed.

In some companies peer review is done by pairing. One person types, the other reviews in real time. Reviews the requirements, security, UX, coding standards. This removes the feedback loop, and encourages greater reflection on each line of code, and the wider context, because the review isn’t coming in raw.

Some companies use a mix. Pair for some, or most work, but still leave space for pull requests so that other experts can review, and feedback, and understand that feature.

But however you do it, the feedback is key. I’ve seen a few people on Twitter talk about pull requests being too late, because of the feedback loop. Once the toast is burnt, it’s useless. Which is valid, but that also suggests to me a culture where the feedback talks about the result rather than the process. “This toast is burnt” is ineffective feedback. It states a fact without providing a learning opportunity to understand how not to burn it next time. And pairing doesn’t always provide that opportunity either. What might look, when copying someone else, like an odd coding convention, might be the difference between a successful release and a crippling SQL injection attack. But you need to ask why.

Some people, especially devs, are far more comfortable asking why on their keyboard, when they can reflect themselves, and research the concepts. Equally reviewers tend to be harsher in the pseudo-anonymous situation of conversing with a code diff, than talking to someone face to face. Reviewers have got to the stage of telling me they feel guilty about comments and then apologising face-to-face, but standing by the desire to see the best possible code committed.

Yes, there are definitely problems that shouldn’t make it as far as a pull request, but if you have those problems, ask yourself why there’s such a big gap between code writing and code review, why your mentoring process didn’t pick up on that issue, if the feed on process to Dev needs its own review. My experience, if something is broken in the pull request code, it was broken before that code was written. And you need to teach that developer how to use a toaster the way the rest of the team does.


 

If you want to continue this discussion, there’s some great threads to follow here:

Advertisements

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s