When working by ourselves, we usually push directly to our project repository. We might have two separate branches: one for adding (experimental) new features, and one more stable to use for deploys.
With a two people team, we might continue to use the same setup. It might work well if for example we are both strong communicators and keep each other well informed or if we share the exact same vision and values.
As the team grows, it gets harder and harder to bring everyone on the same page and to keep track of the changes introduced by each member. There are many ways to solve a particular problem, but some solutions we might see as better. For example, they would be more suitable with the overall architecture, easier to understand, or would lend themselves better for future extension. Other solutions solutions would need improvement - for example, they might be incomplete, brittle or less extendable.
Either way, with a larger team (really any team with more than one team memebrs) would benefit of code reviews.
Each developer on the team would create their own branch, add their changes there, then request a review from their peers, and only once they receive an aproval from a certain number of peers, the changes can be merged into the main branch.
Code reviews help:
- to improve the quality of developers' work
- to improve the quality of the code base
- to stay on the same page, and be aware of new changes introduced
- to serve as documentation
Make it official
It is important that code reviews in your team are formally established and there is a (sort of) process how to do code reviews.
If a team is small, it might seem tempting to have just-ask-for-review policy expecting team members to request a review when they think it is needed and then have an informal chat, for example.
It becomes more difficult for juniors and newcomers to determine what constitutes a piece of code that is worth requesting a code review.
In addition, informal chats without any written comments will leave the team with no documentation of the review for future references.
As part of making it official, it is important that all stylistic choices are automated with tools like: Prettier, Stylelint, Eslint, editorconfig, to name a few. This was the focus of the review can be the implementation.
Formally established would mean the entire team is aware that code reviews are a step of the process of releasing a new feature and they should be factored in when estimating how long it would take to complete a feature.
If code reviews are not formally established, it might be difficult to make the time to review other people's code. Time is essential, when team members are expected to put their best effort when reviewing.
As contributor
Add context
Make reviewers' lives easier and add the necessary context to your PRs. Context should be something that hints people towards what you are talking about. Stylelint has a great example of providing context for its rules:
Be careful though - providing context should not be falsely guiding. For example, for UI changes some suggest adding screenshots or short videos. This might be very helpful if its purpose is to provide context. However, you should avoid using this technique as an evidence of working code.
Don't go rogue
Pull requests for tasks noone else knows about is something to be avoided (even when there is context provided). Upcoming issues should be first discussed with the rest of the team, clarified and scoped, prioritized, possibly assigned and only then executed.
Since one of the agile principles is continuous delivery of valuable software, it is important that the team evaluates the tasks that whould be executed next.
In addition, it prevents lack of transparency and a culture of cherry picking the fun tasks.
Keep PRs focused
Ideally a pull request should be solving one thing at a time. If a pull request solves multiple and unrelated topics, it becomes harder to review and consequently to merge. The amount of changes might be too big to review, which would lead to only briefly skimming through the code and letting it pass without a thorough check. The alternative - a thorough review, also would not yield much better results as it might take significanlty longer until the changes get merged.
It might be a good idea, that each new set of changes is linked to a particular open issue (on a issue management board, like Jira, Github Projects, or even Trello). Each of the issues, should be clarified and scoped by the team, before any work has been done.
Code review should be a conversation
When your code has been reviewed and there are several requests for changes, most likely one would start fixing them one after the other. What people often don't realize is that some of these comments are merely suggestions (even though they might not be formulated or labeled as such). It is perfectly fine to question these suggestions a little and try to understand what is the reasoning behind them as well as whether they are nice to have or must have. The nice-to-haves could then be extracted into issues to be fixed at a later stage, and often make good first time tasks.
If the conversation tranforms into a long discussion, this might be an indicator that the team is not on the same page. Pull requests caught up in debates become harder to merge, as they become stale or even obsolete. Ideally, discusions should happen before work has already been done and pull requests should be solutions to items previously discussed and agreed upon by the team.
Ask for a review before you are done-done
Many times pull requests might feel as bar to be passed if you want your code merged.
Pull requests though can be ideal for receiving feedback on your implementation of a task.
As soon as you have something working, not necessarilly perfect, you can open a pull request and label it as work in progress, then tag few people to see that they thing of your implementation so far.
This way, if the implementation is going in the wrong direction, it can be corrected much sooner.
In addition, pull requests become easier to digest, and communication within the team improves.
As reviewer
As developers we identify ourselves with the code we produce. This can make it quite difficult to accept feedback that our code needs improvement. As a reviewer, your task is to contribute to a improving the quality of the codebase, but also to help others grow.
Don't assume - ask questions
When reviewing a pull request, avoid making assumptions and instead make sure you ask questions.
If you are puzzled by a certain choice or if something just doesn't make sense to you, you can always ask for details: Hey I was wondering what is the reasoning behind this decision?.
When you ask a question instead of making a statement, a person is more likely to give you an explanation, while retaining positive attitude.
Sometimes it will turn out that there is no reason in particular and the other person would be happy to make a change. Sometimes - there will be a good explanation. Either way, communication is the key, so you both continue to feel valued and heard.
Be nice
This might seem too obivious, but I believe worth mentioning.
We are all busy, and in our daily rush it might seem like a good idea to just leave quick comments such as: Fix this or Doesnt work or Here too and tell our team members to not take it personally.
This might be a perfectly functional approach for some teams but I believe it is worth to make an effort to be polite and even make complete sentences.
When we just see a message on our screens, it is devoid of sentiments.
We can read it and attach to it an imperative note, dislike, annoyance, or anything really.
On the contrary, a complete sentence like: Hi, this looks great! I would suggest we add data caching to improve performance. What do you think? leaves no doubt the author is being friendly and open for questions and collaboration.
Review other people's pull requests
It might be a good idea to review other people's pull requests even though you have not been assigned as a reviewer.
It is interesting to see how people approach a problem and go about solving it. This is a great way to learn not only about team programming techniques, but also about a new codebase or parts you have not worked on. Once you decide to review a pull request though, you should be commited. If you do not plan to do a complete review, make this clear. This approach also enhances the feeling of shared ownership - it is everyone's job to deliver working software.
Balance is key
Sometimes in a team there is an imbalance between people who get reviewed and people who are reviewing. Maybe it is always the most senior or lead who is doing the reviews, and junior members never get to do reviews. Maybe the one who has been on the team the longest always has the final say. In a healthy team everyone should get a taste of both roles. It prevents senior developers from feeling infallable and becoming insensitive. For junior members reviewing code is also a way to grow, reading possibly more complex code.
Conclusion
Code reviews are a great tool to maintain and improve code quality and to help team members communicate better and grow further.