6 Signs Your Code Reviews are Toxic and Poisoning Your Team
I’ve noticed a growing trend over the past 3 years or so, the growing prevalence of code reviews. I’d been participating in code reviews in a haphazard manner since I personally entered the industry in 2003. Back then you’d print off some code and mark it up with a red pen. Nowadays, tools like Stash and Crucible make it extremely easy to do code reviews. Making code reviews part of pull requests has really pushed us all forward in this regard.
Warning: This is all IMHO, continue reading at your own risk! I think everyone would agree this is a “good” thing. What I have noticed is that some organizations and teams have cultivated truly toxic code review processes. So bad, it drives away good developers. To illustrate I’ll parody a code review happy path and a code review toxic path.
Code Review Happy Path
You’re new to a team or company. Maybe you’re entry level or you’ve been in the biz for a few years. Whatever, you complete a couple of features, make some pull requests, set your lead or seniors as reviewers. They review your code, and in the process you get a feel for what they’re looking for. You incorporate their feedback and your reviews become a formality going forward. You’re dialed in and start cranking out features and code. Occasionally bugs, errors in logic, or possible security vulnerabilities are noted and fixed.
As part of the happy path, maybe they have linters and findbug utils incorporated into the build to enforce things like group formatting standards (tabs vs spaces for example). Maybe there’s a coding a standards document that talks about things like naming conventions and appropriate levels of abstraction. Great!
The main feature of the happy path is; everyones happy, the codes good and the team is productive. This is what EVERYONE wants. The team is capturing the spirit of what code reviews are intended to do: improve software quality.
Code Review Toxic Path
As a new developer on a new team, unlike the happy path, after your first few pull requests and code reviews, you don’t reach a comfort level of the local standards. You’ve incorporated prior feedback, and every code review presents another reviewers nit pick of the day. There’s no linting or other enforcement in the build, and no coding standards so you have nothing to go by. Reviewer Sally prefers nounVerb naming, Reviewer Rajesh prefers verbNoun. One day Sally decides that all JUnits with Assert.assertAAAA() should have positive boolean logic as the args, so now some are Assert.assertTrue(), and some are Assert.assertFalse(). This may sound absurd but I saw this once.
The main feature of the toxic path is no one is happy and development velocity is slowly crawling along. Code reviews take forever and the team is slow to deliver features. The software delivered has obvious bugs. Personally, I’ve quit teams and managers that allow this.
Warning Signs
Take a few minutes to check the health of your teams code review practices. If your code reviews exhibit any of the 6 qualities below, you probably have multiple problems that are costing you development velocity and developer loyalty.
- Arguing over formatting. Things like tabs vs spaces and brace placement same line or next line should be settled and enforced by the build.
- Reviewers competing for the most clever one liners or ever more layers of abstraction and indirection. Code should be maintainable and the appropriate level of abstraction. To me this means the minimum level of abstraction. This is obviously a contentious point. Talk it out!
- Arguing over variable, method, class, or package names. Decide on a methodology and stick with it.
- Reviewers requiring weird things like the Assert.assertTrue example above. Ideally all standards would derive from industry standard norms, like SOLID principles. If a reviewer can’t express something in those terms then it shouldn’t be a coding standard.
- Previously unstated requirements are presented as code review required changes. Missed requirements ought to be an enhancement for the next sprint.
- Reviewers requiring changes in code that’s not part of the review. Code beautification and refactoring ought to be tasks in the next sprint as well.
In short, if a coding standard can’t be enforced mechanically (ie with a linter in the build), citing an industry standard, or referring to a group wide standards document it ought to be disallowed from a code review. That way we can all focus on finding bugs, logic errors, and security vulnerabilities.