Why do we do code reviews?

We have four main reasons for doing code reviews:

  1. Increase code quality
  2. Learning
  3. Context and knowledge exchange
  4. Find bugs sooner

1. Increasing Code Quality. We want to deliver high quality software. Some of our sites are very important to lots of people. They need to be reliable, stable, maintainable, testable and visible. No developer is perfect, but everyone is different and has a unique perspective. Code reviews enable us to discover possible bugs or architectural flaws early on, before even manually testing, QA, and delivering the site.

2. Learning. As developers, we never stop learning as our programming languages and concepts constantly evolve. Code reviews are an important part of that learning process for both the reviewer and the reviewed person. The reviewer often discovers new ways of solving a particular problem while reading through the code or asking questions. The reviewed person receives important feedback and valuable suggestions on how to write cleaner or more efficient code.

3. Knowledge Exchange. Code reviews are also a great means of project- and company-specific knowledge exchange. When developers get onboarded on a new project, code reviews help them to understand the structure of the code faster. They implicitly learn how “things are done” in this project. Often we have important guidelines written down in our documentation, or a Readme file, or something similar—code reviews will speed up the process of “diving in”.

  1. Find bugs sooner. If we can spot a bug before it is noticed by a client, everyone wins.

What is a code review at an agency

How do we find a person to review the code?

  • Ask in a Slack channel or whatever office communication tool you have.

It is up to the person needing the review to determine the checkpoints for reviews. It is recommended that there be at least 2 checkpoints for small projects (at ~25% and ~75%) and approximately 1 per 2 or 3 weeks for larger projects.

A review request consists of:

  • Before the review, clean up your code. Lint all the code. Also, add comments, remove debugging code or other experiments, and generally get ready to explain what’s going on.
  • Provide context for the reviewer:
    • Code being reviewed - what is it doing?
    • Code structure. What are we doing vs. what is in a third-party library? Are external elements or APIs involved? What are key folders and files to
    • Description of the project (this should also go in the README)
    • Suggestions - Note what you think the reviewer should look for. Some complex code? Something that isn’t performing well? A part of the code you struggled with
    • Screenshots of the output if necessary
  • The staging site (if available) or an Ngrok URL (discuss times to make this available to the reviewer)
  • A “review” is a text document in the repo. It is a list of links to code in the repo (on Github, Gitlab, Bitbucket), or a list of files and line numbers.
  • A suggested date for discussing the review after it is completed by the reviewer (30 minutes max)
  • After the review is discussed, it is up to the person being reviewed to assess how many of the comments to act on. At the very least, the person being reviewed:
    • should address any security or performance concerns
    • should pick out a few (at least 1) things to improve on. Try to learn from every review.
    • check with the project or task estimates and see if there is time to incorporate the other changes

Responsibilities of the Reviewer:

  • 45 minutes to review the code. All your attention during this time.
  • Provide comments as needed. Use the “Conventional comments” format below. Be kind. Try to make the other person better.
  • Make yourself available for a follow-up meeting to discuss.

How comments by the reviewer should go:

Conventional Comments

1
2
3

<label> [decorations]: <subject>

[discussion]

Here are the labels we use:

  • nitpick
  • suggestion
  • issue
  • question
  • thought
  • chore

What to watch for

  1. Too much code or too little. We’re not sure what the limits are at this point so this is a work in progress. If we have too much code to review, the reviewer won’t be able to get through it all in 45 minutes, and the risk of missing something important is high. If we don’t have enough code to review, the feedback will be minimal and not as useful.
  2. Over-engineering. Clever code tricks, lots of abstractions, too many systems. These all make the review difficult and might be a problem in the long run.
  3. Pride. We all want to be proud of our work and most of the time this is a good thing. However, if you are too attached to what you wrote, the review could seem hurtful or offensive.
  4. Showing off. Either the person being reviewed in the code selected, or the reviewer in the comments could show off what they know. This isn’t very productive. Our goal should be to get better, not to seek out affirmations of what we already know.
  5. Showing weakness. As a reviewer, if you don’t know what’s going on the code, there might be a tendency to either accept it or skip it altogether. I truly believe asking questions is a sign of strength, not weakness. Asking a question, even if it’s about something small, has two benefits. First, the asker gets an answer. Second, the answerer get’s cement the thinking for themselves by explaining it to someone.
  6. Worrying about annoying or offending the other person. Sure, this could happen. However, by instituting for everyone, and by putting in guidelines for providing feedback, we hope this won’t happen often.

Best Practices for being a Good Reviewer

A code review only achieves its goal when both sides are working together. Here are the guidelines that you should adhere to when being in the role of the reviewer:

1. 🧩 Understand The Code Structure!

If possible, make yourself familiar with the project and the code base before reviewing code. If you’re new to a project, ask a colleague developer give you a general overview and introduce you to the core concepts. This will help you understand the program flow better and make it easier to spot structural flaws.

2. ❓ Ask Questions!

Do not let your fear of showing weakness take over! If you don’t understand a piece of code, don’t just jump over it in the assumption that the developer who wrote it probably knows their stuff and you’re too stupid to see that. There are two possible scenarios:

  1. Your fear turns out to be true and the implementation actually makes sense. In this case, you’ll learn something new from the answer you receive. You’ll become a better developer step-by-step.
  2. The code is really illogical, incorrect or difficult to read. In this case, it’s important to fix the problem or make the code more readable.

Each of the two cases satisfies one of our goals. Not asking means missing an opportunity to grow or missing a flaw in the code.

3. 🙋 Speak For Yourself – Not For Others!

Feedback is always subjective. Every developer will comment on different things in the same batch of code. There are, of course, many issues that most experienced developers would agree on, but there is no absolute truth.

There is one simple rule to prevent this from happening:

Always speak for yourself and formulate feedback from your own perspective.

Instead of absolute claims like “this is messy code” or “you can’t do it like this”, use personal and constructive sentences like “I have difficulties understanding this code” or “What do you think about changing it as follows?”

It’s much easier for us to accept and think about feedback when it’s formulated as a personal opinion because we know: Every person has the right to their own opinion, but no-one can claim the absolute truth.

  1. 👁 Focus on The Right Things!

Refrain from adding comments for things that are clearly personal preference. Let a linter take care of formatting if possible or else, try to not let formatting comments distract you from discovering real bugs. Here’s a short (non-exhaustive) list with suggestions on what to look for:

  • Single responsibility principle / Separation of concerns: Is a class or function taking care of too many things at once?
  • Naming: Is a variable’s, function’s or class’ name expressive and precise enough that it’s easy for you to understand its purpose?
  • Hierarchy & data flow: Is it difficult to see how the data flows due to a missing hierarchy?
  • Imports: Does a class import a library that it shouldn’t be concerned with? (For example, a networking class normally shouldn’t know a thing about UI components.)
  • Edge cases: What happens if something unexpected happens in the program flow (like an interruption of the network connection, an illegal string that the user entered etc.)? Are errors handled properly?
  • Code Repetition: Is the same or strikingly similar code used in multiple places?
  1. 🖍 Clearly Mark Optional Changes!

Depending on the project, decide for yourself if a comment you added is crucial to be discussed or if it wouldn’t hurt to keep the code as it is. Agree on a word or a symbol with your team to indicate optional changes of little priority. We prefix comments for optional changes with the word nit (short for nitpicking). You may also use an emoji like this one: 🤷‍♀️, or any other symbol.

Marking optional changes speeds up the process of adaptation as it delegates the responsibility for whether or not to perform the change back to the author of the code. This best practice helped our team to avoid long discussions about things with little relevance or personal preferences.

Other notes

  • If we’re in a rush, we can confine the reviews to be within the team. This should save us some time creating the context for the reviewer.
  • We don’t want to stop code from being deployed. The vast majority of our code is good shouldn’t be held up. It’s okay to review code that’s already been deployed.
  • There is a version of a code review that is just a quick check. If you need a specific feature or function checked and don’t need a full review of what you’re doing, that’s completely fine to do. The code you need check still needs to be prepared as if it’s a full review, but the quantity of code and the scope can be smaller. If you need a quick check, you will still need to set time limits, and arrange the schedule with the reviewer.

Some of this comes from this article:

Code Reviews Done Right: Your Missing Guideline - QuickBird Studios Blog