More about code reviews

Many thanks to @peitor for engaging with the last code review post, particularly his comments there, which I recommend reading, and his tweet:

@craignicol @yochum Additional to what CodeReviews are for: team building and finding alternative solutions.

— Peter Gfader (@peitor) October 1, 2015

I want to explore a few of his thoughts a bit further, particularly around what code reviews are for.

“team building”

You need to build your reviews carefully to allow team building. As a technical lead, I always get a member of the team, or a peer (if there’s no team yet), to review my code. This can be intimidating for people who believe that there is a hierarchy of knowledge in the team, where the lead knows all, and imparts knowledge. How can such an Oracle be challenged? If I can’t be challenged, I’ve built the wrong team. No-one should release code without review, especially the technical lead, who’s likely to be doing the least coding.

In a properly functioning team however, delegation means that the lead doesn’t have to know everything. They can rely on the developer doing the work to understand the process better than anyone else on the team, and use the review as a learning process to share that knowledge. A functioning code review process not only promotes quality, but it promotes communications within the team.

“finding alternative solutions.”

This is a great use of the review-as-pairing model where the code review is started before the code is complete, allowing for a discussion of options, and the wider context, to ensure the most suitable solution is found.

Maybe you just think of this as a chat between developers, but it’s a great time to review the code and the ideas that generate that code, whilst they’re easier to change. Much easier to change a thought than a test suite.

“What tool can we leverage to make the review more automated?”

I would argue, as @michaelbolton so eloquently does when discussing automated testing with John Sonmez, that the things worth reviewing are the things you can’t automate. Click through and see his replies and the blogs he links to. It’s a gentle but powerful argument.

@michaelbolton Uh oh, someone on the internet thinks I'm wrong. 😉

— John Sonmez (@simpleprogrammr) February 7, 2015

Tools are great. I love compilers, static code checkers, I love the Roslyn examples I’ve seen, but all that comes before the code review. If it doesn’t compile, or it doesn’t meet the style guide, or the tests don’t pass, it’s not ready.

That’s not to say it can’t be reviewed. There may be questions that need reviewed and answered before all the automated stuff passes, but the sign off review requires that the change has passed the automated steps before it can be reviewed.

Also, be wary of following style guidelines. There’s a reason compilers don’t complain about these things. Unless you know why a guideline exists, don’t follow it blindly. Review your automation as much as your code.

“Review not only code in your team. What about the build process? Deployment scripts? Configuration setup?”

Definitely. This might need to involve the whole team, but everything should be open to review and reflect, and where possible, version it so you can review, share and rollback changes. Don’t trust change, but understand it and use it to help you improve.

“Does it stand on its own?”

“What do you mean by stands on its own?As in, the code under review is a complete new feature, or that it is self-consistent (code and tests match, etc.)

Does the change need a lot of explanation via voice? Or is everything there, so that a future reader can follow everything aka the Why? What?”

Does the code leave behind enough context? We all know the scenario where we’re trying to track down some obscure bug, and then we see that 18 months ago, someone used a > rather than a >= and you’re not sure why. The code review is a good chance to document those decisions. If you want to make sure everyone knows you meant “tomorrow or after” instead of “today or after”, make sure it’s explicit, so that when someone calls your code in 12 months time, they don’t get surprised.

Is there anything else I’ve missed?

I love reading your comments, so please let me know

Advertisement