The Experience of Code Review at Fiverr

Hey! My name is Inna and I’ve been working for Fiverr’s R&D office in Kyiv for the past two years. In this article, I will share my observations on the code review process at a company with ~25 development teams. We’ll talk about the approaches and tools we use, as well as the problems that can arise when ~150 programmers work on one product.

Generally, management of many is the same as management of few. It is a matter of organization.

Sun Tzu

Illustration by Jane Linville

Intro

Fiverr’s R&D is divided into small teams; each one is responsible for a separate part of the functionality (payments, gig page, or pop-up registration). Such teams are called Task Forces (TFs) and consist of various specialists: developers, product owners, product managers, and sometimes designers and business analysts. These TFs form verticals, or independent units of 5–10 people. Working within these teams would be difficult if there were no common mindset.

Why do we review code?

Basically, code review is a way to share a common understanding between dev teams, based upon the following principles.

• consistency

If each developer wrote new pieces of code in their own style, it would significantly complicate the project. The code would be less readable, and developers would spend more time trying to understand it. However, by implementing standards, you can achieve consistency in pattern design and the architectural approach even in the most remote parts of the platform’s code base.

• teach & learn

Code review is probably the most effective way to share knowledge between developers. Courses, books, and tutorials promote professional growth, but during review there are at least two people who exchange ideas and share their impressions of the code — even if it’s a little thing like “5 more ways to align the div in the center.” When writing the code, I also try to find optimal and interesting solutions that can be useful for my reviewer.

• code quality

Code review does not help avoiding bugs in the production environment completely. Occasionally, there’s a gross error in the code that the reviewer notices, but often bugs arise from the small flaws. The task of the reviewer is rather to make sure that the code meets the standards, as well as being truly useful, readable, and consistent. They can advise on how to improve it: consider edge cases and offer tests or fitting patterns.

Code review platform

We use GitHub in combination with various automation tools for Slack. More personal communications also come in handy (Zoom or offline in the office).

In the second part of the article, I’ll first describe the problem, then you’ll have an opportunity to think about the correct way to solve it, and then we’ll go over our solution. I’ve chosen five issues that are specific to teams of our size and/or just seemed interesting to me. If you have other solutions to the problems I’ll describe, I’d be happy to read them in the comments.

Problems (and solutions)

1. Horizontally, not Vertically

In theory, all of our teams are vertical and independent. However, we are working on the real product. Therefore, they are independent as long as there is no need to interact with other teams (surprise). For example, there is a logged out homepage — a page that is displayed to unregistered users. A separate team is responsible for it. This page is isolated because it does not require user data or a payment session.

The domain of Logo Maker is not so isolated: there is a page for registered users, and you must initialize a payment session and display a beautiful order page. Therefore, the code review must be performed by other teams. This can be someone in a different time zone, someone with a holiday, or it can be somewhere Friday is a day off. And pull requests can get lost.

Small bureaucracy to the rescue

I’m sharing this from the experience of teams whose repositories often get requests for changes. In simple cases, a Slack channel (kinda like #team-name-prs) is enough. Everyone can send a GitHub link here with a brief description of the required changes.

When this is not enough, and the PR begins to get lost in the flow of messages, we create a separate board using Monday. Monday allows you to add a PR through a special form and assign different people to review the part of the system with which they are more familiar.

There are 7 developers on my team, but we rarely have external PRs. However, we also decided to try using the Monday board. Our internal PRs have become more transparent, our reviews more active. And after the weekend, there is no need to look for skipped PRs.

The board also helps to handle prioritization: you can see the whole scope of PRs at a glance. One PR could be of 200 lines but can wait; and another one could be 10 lines but a very important fix.

2. Diversity

Teams may have their own approaches to development, their own traditions and rituals for writing code. Well, nobody’s perfect.

Standardisation

Our onboarding lasts 2–3 weeks and includes getting familiar with code review documentation. New developers learn how they should write code, how it should look (guidelines for code style), how to build the architecture, how to name PRs, etc. Perhaps having a common mindset is the most important aspect of efficient development.

In this way, when you work with a different team, there’s the same indentations in files, React-components are written with a similar principle, the architecture of an application is the same, and routes are written in snake_case.

3. Alien

Sometimes you get a task and do not understand how it will affect another team and their ownership. You can just read the code and try to add your changes to the code base, but they still need to be tested. And what do you do if you don’t know all the logic and there’s something complicated? And maybe you don’t even know the language. Okay, I’m already exaggerating, I just want to talk about how not to fail in someone else’s domain.

Encapsulation

The safest way to make changes to another domain is to isolate them.

“We get a dozen PRs every day and we all need to make sure that people follow the guidelines and use our infrastructure; otherwise, everything will quickly turn into a mess. To maintain the consistency of our own domain, we ask developers to make their changes in separate packages and then inject them into our repository. This makes it easier to track changes and increases code readability. At the same time, the package remains in the ownership of another team; here they only update the version of the package.”

© Chelli Kircshenbaum, Dev Team Leader at Fiverr

This approach is not always possible, but if possible, it should be followed.

Design review

Another approach that allows you not to break something in someone’s domain is to conduct a design review (DR) in advance. We have the DR in the format of a presentation. The person gathers a circle of interested people who are best versed in the subject area and brings up the problem for discussion, and then they suggest ways to solve it.

First, it helps to avoid PR deviation even before one line of code is written. Second, we avoid incorrect architectural decisions and immediately lay the groundwork for proper implementation.

“Even if the changes are encapsulated, DR is a must, as we are responsible for the reliability and stability of our domain. Almost every feature goes through a DR or at least a short sync. Its absence is a red flag for me, as we are not aware of the proposed changes and have to undergo DR afterwards. Sometimes people don’t have the best solutions because they may not know all the features of our domain and don’t consider that their changes can affect performance or create bugs.”

© Chelli Kircshenbaum

Of course, if it’s a minor change, a package update, or if everything is clear from the code, DR is not required.

4. Time

Code review is an integral part of the job, but there is not always time for it. And while the reviewer is trying to complete their own task, the PR is becoming obsolete, there are merge conflicts, the developer has already forgotten what they wanted to achieve there, and so on. After the review, it will be necessary to update the PR, resolve conflicts, test it again … it’s a waste of time.

Automation

We are trying to automate where possible to save developers time on routine work. In addition to the familiar circle CI, which drives tests, builds a project, etc., we have some other interesting things.

For the most “popular” repositories, GitHub automatically generates a checklist in the PR description. With its help, a developer can check themselves. This is very convenient because otherwise the reviewer would have to write the same thing several times a day to different people.

Our repositories have a CODEOWNERS file. GitHub can take the team’s name from it and automatically assigns it as a reviewer. It’s just a trifle, but it saves you from having to look for someone to test the code yourself. There can also be files from different code owners in the same repository, and then the reviewer would need only to view the part of the code that is related to their domain.

5. No QA specialists

It’s pretty easy: we do not have QA specialists (except for the mobile team).

Ownership

But we do have ownership, which means that developers are the “owners” of their domains.

“I like working with development teams that are fully responsible for their domain, because of how they review and deliver their code. In such a team, I can fully rely on the fact that the developers will maintain a high level of code, and this will affect the quality of the product itself (which worries me the most). After all, this is not just an investment in code that is convenient to work with and add new features. It also improves UX because it reduces the risk that the code may break in some user scenario.”

© Sharon Wolfson, Product Manager

With this approach, I realize that my code is what will be in production after the deployment, and it is me who must make sure that nothing falls apart. There will be no more testers who check the correctness of the domain after merging code.

“There is a big plus to this approach, because usually the developers have the greatest expertise in the feature — how it should work, so it’s easier for them to test something. Of course, we must not forget about edge cases, not just the happy path to check in prod.”

© Yuriy Dadichin, Former Team Leader at Fiverr

If the problems topic is more or less clear for now, let’s move on to the third (and the last) part, in which I want to share various details about the code review culture or, what I call it:

Social contracts

which I observe every day: from both the developer’s point of view and from the reviewer’s point of view. Because code review is always about two people.

A. Mindset of the one who opens the PR

Responsibility

as a consequence of the ownership.

“We try to minimize the divisibility of responsibility, ideally it should be one person. This is very useful for developers. For example, if there are QA specialists in the team, then they become responsible for what’s getting deployed to the production. Therefore, if a bug goes to the prod, it will be their fault. This approach creates negligence.”

© Yuriy Dadichin

Testing

Before sending a PR for a review, it must be checked on the staging env (fortunately, we have a lot of them) / locally. Even if it is a change of text. Even if just one letter has changed. One day I almost broke the header because the text on the small screen was so big that it went beyond the universe. Thanks to the responsibility (really, a useful mindset), I still decided to test the changes and saw that it was just a disaster. It is unlikely that even the top reviewer would notice this from a single line of css in my PR.

“The non-QA approach is very interesting and useful. Developers feel more responsible for what they deliver to prod. They are the owners of their features and therefore invest more time in the quality and reliability of their code: add unit tests, integration, take care of the quality of their domain, set up monitoring, and more.”

© Michelle Fried, Dev Team Leader at Fiverr

Automatic checks

Circle CI does them after opening the PR. It checks the code for compliance with the standards, launches translations, checks whether the code is valid at all, whether there are tests, and so on. All you have to do is make sure they are successful.

Simplicity

In my team, there is a limit of 200 lines per 1 PR. We adopted this rule recently, but we are already very happy with the results. It is even physically easier to review code of 200 lines three times than one of 600 lines. The number of conflicts also decreases, so something can be resolved faster. On behalf of our team, we highly recommend doing small isolated PRs.

Details

A review always distracts from your work to see who wrote what. In my opinion, one of the indicators of high-quality PRs and genuine concern for colleagues is the approach by which the developer tries to minimize the time spent by the reviewer to search for mockups in Figma, product requirements, and so on. The description of a PR should include answers to all of the questions that may arise from the reviewer: how it works (business logic), what it looks like (examples with screenshots, videos, or gifs), what it affects and what problems it solves, etc. You can also add comments in the code that explain a decision, as well as add a link to an article explaining the approach.

Monitoring

responsible people monitor the state of their system constantly. We use Graylog and Coralogix for logs, graphs are built in Grafana, . Monitoring is ongoing, not just after deployment.

In our domain there is a special channel in Slack, where aggregated error messages come from. They are divided into backend and frontend. Track system load, response time, number of requests and errors on endpoints and pages. Indirect business KPIs are also tracked: number of orders, homepage visits, projects created, etc. If you exceed the set thresholds or unexpected changes in performance, a message arrives in Slack.

Monitoring helps us to be confident in the stability of the system at any time. As Yuriy, my teammate, said, “I will not leave the house without such monitoring.”

B. Mindset of the PR reviewer

Prioritization

We try to review PRs as soon as possible after they are opened. This way, the reviewer will have to spend less time merging with the main branch, running circle CI again, and testing again.

“People regularly check and review their and others’ PRs, at least on the same day, or in a few hours. This is an integral part of the job, the same as developing a feature. No one worries that you need to review PR instead of delivery.”

© Yurii Kabai, Dev Team Leader at Fiverr

Simplicity

The reviewer should not comment on code that could be improved in parallel *. It suffices to leave a comment that it would be cool to fix but not critical. I read that some companies use the prefix nit (nitpick) for such cases; we need to introduce this too. After all, the purpose of a review is not to perfect the code, but to improve it to a certain level.

* — this does not mean architectural changes (when you need to change something in the existing code because the logic has changed in general) but simply a diff on GitHub that shows a few lines at the bottom and top, and sometimes you see something wrong that gives you the itch to ask for a fix.

Benefits

A review is an opportunity to learn something new. If you approach it this way, and not skeptically, you can pump up your skills well just by looking at the code. Sometimes I read PRs of teams that have nothing to do with me at all. But they were written by cool specialists and I can almost always spy on interesting tricks in the code.

He/she/it/them/us

That is, not “you need to rewrite,” but “we need to improve.” The wording sounds like banal politeness, but it’s a little better. In a PR, there is a discussion of specific code, not the abilities of individual authors.

“It is not enough to say “this is not right”; you need to explain to the person what the problem is, and you should always pay attention to your manner of communication, because there is a fine line between what will be useful and what can be perceived as a personal insult. Comments must be relevant and always relate to a specific implementation.”

© Oded Goldglas, Tech Lead at Fiverr

A good gesture is approval when some little things remain to be fixed. I trust my colleagues such that they will be able to resolve the rest of the comments without checking with me.

🌿💥🌸🐝

If everything is so cool that you can’t say anything, and LGTM sounds too formal, then in PRs we put funny emojis under the approval. It seems that we have literally several teams doing this, but I’ve really liked it since the day I got my first one.

“This is a silly but positive approach that adds to the mood of both review participants. The review shouldn’t scare people, it’s just an opportunity for everyone to make our code base better, and such gamification makes routine work just a little more fun.”

© Oded Goldglas

Summary

or TL; DR. I could have inserted it at the beginning, but who would read everything then?

The following approaches help us to help many teams work together and make effective review code.

1. Ownership: teams fully maintain the stability of their domain / specific feature on their own.

2. Automation: customized linter, circle CI, and checklists help developers focus on implementation.

3. Unification: we reduce everything to one style to lower the threshold for developers to enter new domains.

4. Prioritization: do not hesitate to spend working time on a quality code review; minimalism: a minimum of files, a minimum of lines, remarks — only on the case.

5. Responsibility: testing the code on the staging, monitoring the graphs on the prod after the deployment.

6. Communication: maintaining an atmosphere of friendliness and mutual respect is more important than your own preferences for writing code.

Fiverr is hiring in Tel Aviv and Kyiv. Learn more about us here.