Things Code Reviewers Hate

Is reviewing code part of your daily job? Unless you’ve got a super nice team (that doesn’t actually exist), you must have faced most of these aspects while reviewing your peer’s code. Although I’d completely agree that I must have submitted this kind of code when I was junior, hence the karma!

On average, I review about 6-8 pull requests daily. Some PRs require my approval because of Github code owner rules, while the rest of them are raised by my team members. Every few days, the developer inside me is hurt because of the code I get to see. Few of these points are related to pull requests, while some are generally related to writing good code. These are the top few things I hate:

1. Long pull requests - There are plenty of reasons why our PRs should be short but here I’d share my perspective as a code reviewer only. When I look at long PRs, I’m not able to understand what we’re trying to do as a whole and why did we not break this change into parts. Let’s agree for a second that this PR is doing one dedicated task so it should be okay to accept this, but if I give 10 CR comments, and the developer fixes 8 of those and justifies the rest two, I’d need to again look at the long PR again to see what fixes were done and how it impacted overall code. In case multiple cycles of review happen, my context for this PR is so large that I’ll be lazy to pick this for another round of review. Even as a developer, you need to test the entire code again because code changes concerning 8 comments might have impacted some areas. So basically, it doesn’t benefit anyone. Although it sounds tough to break changes into parts, trust me, that’s what a good developer is expected to do. Also, please note that there is no definition of “long” PR. As a developer, we should try to break changes into as many shippable chunks as possible.

2. Missing minimum information - PR title and description should mention the brief scope of change. Although this is standard across many companies, still many developers forget to do this sometimes. Asking for a code review without much information is very unproductive for the reviewer. They should not be expected to ping you, find the JIRA ticket themselves, or magically understand the scope by reading the code. At LocoNav, we have a simple convention of putting the JIRA ticket ID in the PR title first, followed by a brief description of the change. Although requirements and scope of change are mentioned in detail on the JIRA ticket, some developers optionally mention a summary in the description section of PR.

3. Untested code - I feel my time is wasted on code review if I see a piece of code with a logical error in it. This means the developer didn’t bother to test the code locally before raising the PR. This is the worst you could do to respect someone’s time. Some developers think we’ll test the code later (on staging environments) and they raise the PR first. But they forget that doing QA and catching such bugs is not the scope of the reviewer (although we do it sometimes). If we approve this code, and it breaks on further environments, we’ll again have to review the same after the developer fixes this code.

4. Non-Linted code - There are all sorts of tools people use for development. From simplest text editors like Sublime and VSCode to complicated and heavy IDEs like IntelliJ and RubyMine. Almost all tools have configurations that you can set up to lint your code properly. In fact, linting should never be done by hand as it is a task that can be easily given to our editor. Common things include proper spacing & indentation, no trailing whitespaces, and no extra new lines. Beyond this, there could be language-specific rules. Please always lint your code properly before submitting for a CR. For me, it’s a turn-off to see non-linted code and I don’t shy away from giving comments like “Please use space on both sides of equals operator”.

5. Repeated mistakes - Unless your organisation has a high attrition rate, or is shuffling people across teams frequently, you must be working with the same set of people for a long time. We know how someone is growing. In the beginning, people do all sorts of mistakes and get many review comments but that generally improves with time. Frustration kicks in as a reviewer when people don’t improve on the same set of mistakes.

The next two points are more related to writing good code, but still listing them down here as they are related to the PR reviewer’s experience:

1. Poorly organized code - We need to ensure that the low-level design of our module is good and that our classes & interfaces are separated nicely. Although this could be a wide separate discussion, we should ensure small classes, each minding its own business and not strongly coupled with other classes.

2. Catching Top-level exception classes - We often write long methods that could throw a lot of exceptions, and later we just wrap the entire method’s code in a begin...rescue (or try...catch) block with exception superclasses (like StandardError or Exception class in Ruby and Exception class in Java). Doing this is so risky that it could be a complete post of its own. This StackOverflow answer describes the issues for Ruby language, but those are conceptually applicable to all languages.

So next time you plan to raise a PR for review, I’d recommend you to consider atleast these points (although there are many more). I prefer to self-review the PR first before submiting it further to ensure that there are no obvious mistakes or debugging messages left. This small habit can help reduce to and fro between you and the reviewer, and improve feature delivery speed.

Thank you for sparing time to read this. See you later!