pancake

Code quality & review guidelines

The following is a document I created at my $DAYJOB.

We are a team of engineers with different backgrounds, ages and experience. To make our life easier, we decided to unify our workflows with git, git forges and tech-focused interaction in general. We have several (around ten) wildly different repositories: different languages (mainly Python and Go) that were created over many years (the oldest being around fifteen) by different teams; the current engineering team inherited them.

The two scariest ones are:

*) at least that's where the git history cuts off, with 20k additions in the "Initial commit" patch.

Tickets and issue reporting

Take your time when writing the issue description.
It is obvious what is wrong when you are filing the ticket, but you can totally forget what the problem was when you get back to it one year later. Include acceptance criteria and the scope of expected work.

Make the issue easy to understand and reproduce.
Describe the error and the steps needed to reproduce it. If it is not obvious, include expected and actual results. Include tracebacks, logs, versions, mention specific code.

Commits and PRs

Every commit should be compilable/runnable/testable.
When making changes spanning over multiple commits, each one of them should pass the test suite, the linting checks and/or other quality metrics.

Force-push and rebase is your friend.
When asked for a change during PR review, update the appropriate commit with --amend and force-push the new set of commits into your feature branch.

Take the time when writing the commit title.
It should be an umbrella for everything the patch does. If there are multiple things that don't match that title, it should likely be split into more commits.

Every commit title and message should follow the Conventional Commits practices.
See conventionalcommits.org.

Include the Jira reference.
It significantly improves the git archeology experience. Something like this:

fix: Ensure --version does not crash

* Card ID: PROJECT-XXXX
* Card ID: PROJECT-XXXX
* Card ID: TEAM-XXX

Explain the reason the change is being made.
Both in code and in the commit description. "Code you wrote six months ago is no different from the code written by someone else." Don't let your current understanding of the problem fool you; imagine a person that follows the project but doesn't actually write code (the manager, PM, ...): they should vaguely understand the change just from the description.

Describe how to reproduce and test the fix or new feature.
Sometimes it is hard to test and reproduce the issue. It is useful to describe how to test new features or reproduce issues especially, when the testing environment is complex. This description should be part of the PR message or the Jira card.

Test your code.
The CI will tell you if an old test failed, but it won't notify you if the test coverage decreased. We don't aim for 100% coverage, but we should be unit testing the implementation. The tests will find regressions months and years into the future, and are yet another way of documenting the functionality.

Keep refactoring and new features separate.
One commit should only do one of those, if possible. If a bigger change requires some setup on semi-unrelated parts of the code, which could be done even without the new patch, they should be done in a separate commit. Very small changes can be part of the single commit.

Separate tech debt removal from feature implementation/bug solving.
If we know about a section of a code that can be improved without affecting the rest of the code, it should be done as an isolated effort (with its own Jira ticket). However, since we usually find the rotting code while working on new features or during bug fixing, the overhead and wait time may be too long; if the refactoring is small and localized, it is enough to do it in the same PR in a separate commit.

Make the scope of the commit/PR manageable and reviewable.
Large changes take a long time to go through. Splitting an effort into multiple commits definitely helps, but if the scope exceeds some subjective "that's quite big" threshold, the reviewer and PR author should discuss whether multiple PRs are a better way to get the code merged. This can help with getting some fixes in while others are still debated about.

Follow the code patterns even if it hurts a bit.
If the section of the code doesn't follow best practices of the language (e.g. camelCase in Python), accept it and make the new code match. If the change to fix it would be small, you may do it in a commit directly before/after. If that cannot be done and you still feel hurt, open a Jira ticket to fix the problem.

Code review

Things to check during the review.
Verify the code does what the PR claims it does. See if the patch follows the practices of the code around/of the project. If the code does something not obvious, make sure it has comments (in the code itself, in the commit message, both) explaining it.

Be the first pre-verifier.
Reproduce the faulty behavior the PR is fixing. Then checkout/build the patched version and verify the problem is fixed. Play a QE role for a bit, see if there are edge cases to the new solution.

Be actionable.
In every comment, try to be specific about what is wrong in that snippet. You may see the problem/solution from a different perspective; both parties should try to describe what they like and dislike about the solution the other party is suggesting. The author should be able to know how to address a comment the reviewer made. Stay constructive.

Be proactive.
When there is a Jira ticket waiting for a review, pick it up instead of your own card, if possible. Everyone juggles things around; unblocking other people generally leads to higher throughput.

Be timely.
Focus on making the review/update loop fast. When the PR gets updated and the card is moved to "waiting for review", try to dedicate some time the current or next day for (another) review round, if you have the bandwidth.

Reach out to the author if you need to.
Most of the discussion about the code should be done through git forge's review system using comments linked to specific lines. When that isn't enough (e.g. there is an team/company-internal reason to do/not do something), ping the author or speak up during a relevant weekly meeting.

Ask for help when you get stuck.
It is fine if you don't know how to reproduce the original issue or have trouble setting it up. You are becoming the secondary expert on this issue by reviewing it, use the situation to learn something new.

Ask for help when you get into disagreement.
Bring the issue up during a relevant weekly meeting, ask another team member who hasn't been part of the review yet, ping the manager. Everything is better than refusing to move and building up frustration.

Don't blame individual people.
Software is a group effort. Everyone makes mistakes; the commit author, the reviewer and the QE verifier may all miss something that may be obvious in retrospect.

The reviewer merges the PR...
...after they don't have unresolved notes. If there are multiple reviewers, they all should agree the code can be merged. The green checkmarks in the UI are a good indicator of that.

Use rebase & merge.
While every approach has its pros and cons, linear history with commits separated as they were written has proved to be the easiest to dig through.