Introduction

We adhere to a simple rule during our day-to-day work: code that hasn't been thoroughly reviewed is not allowed into production.

Reviewed code is almost without exception higher quality and reduces cost due to less required maintenance in the future. This practice saves money because fewer bugs need to be fixed down the line and the code is easier to understand and modify in the future. Knowledge is also shared within the team and co-working is an enabler when it comes to the learning process.

We currently employ two primary ways of reviewing code: Code reviews and Pair programming.

Code reviews

Code reviews are a great way to spot check work for errors, make sure the code is up to standard, and to share knowledge within the team about specific solutions.

They might, however, add a layer of potential stress to a project if the reviewer hasn't allotted time for the review properly, or the author needs to get a MR through quickly in order to continue working. Another problem is that code reviews may happen after any bad decisions have already been made.

How we do code reviews

Good candidates for being reviewed are identified during the start of an iteration or during the daily meetings. When it has been established who the author of a feature is going to be, a reviewer is assigned by the tech lead. Someone with a fresh perspective or relevant experience is usually selected.

The author creates a topic branch that remains alive for the duration of the development of the specific feature. The author starts the implementation work (after an optional Design review) and submits a MR for review when done. Continuous feedback might be required for features that require more code (some say 400 lines of code is the upper limit for a good review).

Any issues that are identified during the review are fixed by the author, and verified by the reviewer, before the branch is merged.

A few tips to make code reviews better

• During a code review we're primarily trying to understand of how a change will fit into the existing codebase and how maintainable it is.
• More specifically, we're looking for:
  • Duplicated code or functionality (or similar code that already exists in the project).
  • Functions/classes that have too many responsibilities.
  • Any unclearly named variables, functions, classes, or modules.
  • Complex solutions, or code that is hard to understand, that can be simplified.
  • Any untested code or edge-cases that need extra testing.
  • Lack of comments explaining edge-cases or reasoning behind non-obvious solutions.
  • Any unused or commented out code.
  • Unidiomatic language or framework usage (e.g. len(queryset) instead of queryset.count()).
  • Be sceptical of abstractions. It is often better to wait for a couple of use cases to materialize before trying a more generic solution.
• Build and test the code before doing the actual review.
• There's no hierarchy when performing code reviews, just a healthy dose of mutual respect.
• Code style should be handled automatically by linters and tests should catch bugs. Don't argue too much about style and highlight missing or faulty tests instead of defects.
• Keep an open mind. Just because something is not implemented exactly the way you'd do it doesn't mean it is wrong.
• It is not the reviewers goal to find something bad, so don't actively look for it.
• Be empathetic, positive, and unassuming. Ask questions instead of making demands ("What do you think about renaming that function to shazam?" instead of "That function should be called shazam.").
• If you're doing asynchronous reviews and find yourself writing a lot of comments or having many questions, reach out to the author and schedule a quick meeting instead.
• Make sure that it is obvious that nitpicks are, in fact, nitpicks.

Design reviews

There is another type of review that alleviates the timing-problem of code reviews and it is called a "design review". Design reviews allow developers to propose a solution and get feedback before any code has been written. It is often a very useful practice that we encourage, but it does not fulfill our rule of reviewing and can not be used in absence of a proper code review or pair programming.

Pair programming

Pair programming -- or social coding -- is usually our recommended approach, even though it is sometimes viewed as being slightly less convenient when compared to code reviews. It is a practice that combats loneliness and distractions, leads to a stronger collective code ownership, and the review step is not only instant, but also baked into the process.

We employ four primary styles of pair programming:

• Driver-Navigator: The Driver is focused on the implementation and the Navigator is there to keep spirits high, perform a real-time code review, brainstorm ideas, and to discuss solutions and architecture. Switch roles often.
• Ping-Pong: One developer writes a failing test and the other writes the code to pass it. Then switch roles. Repeat until the task has been completed.
• Same-boat: A task is broken down into several smaller tasks and the developers collaborate by quickly writing, pushing, and merging code. The communication loop is kept super-tight and is typically held in a Meet, in a voice channel on Discord, or in Slack.
• Strong-style: Summarized by the rule: "For an idea to go from your head into the computer it MUST go through someone else's hands". It is similar to Driver-navigator, but the Driver just types while the Navigator does all of the thinking. Any questions are asked after the session. It is particularly useful during onboarding, but remains useful as it pushes the Navigator to really work on their communication.

Martin Fowler has written a nice article on pair programming that can be used as a reference for further information.

How we do pair programming

The tech lead works with the team to detect good candidates for pair programming, and to assign the pairs that will do the work.

Each individual pair agrees on a plan, sets a schedule, and creates a topic branch to work on the feature. The branch is merged when the work has been completed (if no additional review is necessary).

Things to keep in mind

• Make sure knowledge is distributed by sharing any valuable insights or concepts with the team after task completion.
• Pair programming can be quite exhausting. Take frequent breaks and spice things up by trying a different style every now and then. A common approach is to do a couple of hours of Driver-navigator to get the foundational things right, and then complete the rest of the work using the Same-boat style.
• Make sure you share a common understanding of the task and make a plan together before you start. Set a schedule for the duration of the task and stick to it.
• Be nice to each other and have a good time together.
• Not everyone likes pairing, but everyone should at-least try it once.
• Not all tasks are good candidates for pair programming.
• Be present and participate with your partner. Don't get distracted during sessions.
• Most of the tips from the Code review section are applicable during pair programming.