What I learned after more than 1000 code review

Over the past 3 years I have reviewed more than 1000 pull (merge) request’s. During this time, I learned a lot – mainly how not to check the code, how to make the process less painful, which makes the code of good quality and so on.

Pull request should only do one thing

This is the most important thing to pay attention to.

When doing a code review, you have to keep a lot of things in mind. “What is behind this?”, “How is this consistent with the rest of the code?” and “Will it work well?” Here are just some of the questions you need to answer. That way, when you have a pull request that is trying to solve one problem, some of these questions are easier to answer.

Another important aspect is the size of the pull request. Larger queries require exponentially more time to consider. And when I find out that I need to spend more than 15 minutes on the request, you have to wait until a couple of hours.

Larger requests also contain more errors, so the approval time will also increase significantly. This means that you may have a code waiting for confirmation within a few days. And if your company is flexible, it increases the chances of conflicts, which are very painful.

It’s best to break pull requests into meaningful parts – one request should solve only one thing.

So watch out for frequent pitfalls such as renaming, code generalizations, and the like. Although it is innocent and made with good intentions, it removes attention from the important parts – to improve the quality of the code and reduce the number of errors. Just make your brilliant idea in another request.

Automate as many checks as possible

Continuing the idea that the reviewer should keep in mind many things, including checking the formatting of the code, the availability of relevant documentation, passable tests, and so on. I remember a time when I had to take all this into account – it was distracting and time consuming.

It’s good that there is a fairly simple solution – integrate all the checks into the CI. Then all checks will be started for you whenever someone sends a pull request, and does not allow merging until all checks are successful. As a reviewer, you will never have to worry about formatting again.

Automated Tests help make sure that the author did not break anything, and that the tests still pass. Depending on your approach to testing, this is perhaps the most part in the work of your CI.

Auto code formatting leads to the fact that all disputes around the ideal length of the line or the place of adding a new line disappear. Just decide by the team which set of formatting rules suits you and pass it to the autorformer. This will save you a lot of trouble. If you’re having trouble negotiating the format, see how Google, Facebook, or other large companies do it.

Auto Linter Also very important for languages ​​like Python, where code formatting affects the logic of the code. Most Python code formatters only format the code that they know will not affect the logic. By adding a linter, you will solve all the hidden problems.

Type checks and documentation are also worth mentioning, but the most important thing is to automate the checks that make sense for you and your team. If you think something helps, discuss it and try for a week or two.

Sit with the author to view the code

This speeds up the review process, as you can quickly go through the code with the author and share your point of view. The author can also better explain the reason for his approach, for example, if he has already tried something and why it did not work.

It can also be a great opportunity to communicate more with people, which is very important. When your fellow developers see that you have invested in their personal and business growth, they will appreciate it. For you and the author, this is also a great opportunity to practice communication, which is extremely important for well-functioning teams.

Be careful not to overdo it, though. Some pull requests are too small and too simple to have any kind of effective conversation. In this case, a meeting with the author may be a waste of time.

Be careful when writing reviews

If you are more experienced than the author of the code, you should bear in mind that what you say is important. Well-written criticism can help a developer become better in the future, but it can also destroy someone’s dreams.

I found that it is best to ask open-ended questions – this is not aggressive and even encourages the developer to think critically. Will it take longer than just telling someone the solution? Yes, short term. But in the long run, you help them grow, and they are less likely to repeat their mistakes.

So, the next time someone opens the file inside the for loop, and not in front of it, instead of just pointing it out, ask: “How would you reduce the complexity here?” It will mean a lot.

Add the flag “I ran this code locally”

What bothers me most is the detection of an error in a small request, due to which the functionality does not work at all. This means that the developer didn’t even run the code – probably thinking that this is not necessary, since the changes are so small.
After this happened a couple of times, I added a flag called “I ran this code locally”, which completely solved the problem. I stopped looking at code that didn’t run locally, and people didn’t lie about it. There were no offenses, because we all knew that this should be done.

Bonus: this is our template that every developer should fill out when creating a pull request’a:

Description of Merge Request’a
What’s new?

Implemented by …
What changed?

Changed …
CHECK LIST
[] I ran this code locally
[] I wrote the necessary tests
[] I covered my pipe hint code
[] I updated CHANGELOG
Trello card
trello.com/c

Gotta know about
Is there anything else that should be known?
Any comments on deployment?
Any further documentation?

image

Learn the details of how to get a sought-after profession from scratch or Level Up in skills and salary by completing SkillFactory paid online courses:


Read more

  • The coolest Data Scientist does not waste time on statistics
  • How to Become a Data Scientist Without Online Courses
  • Sorting cheat sheet for Data Science
  • Data Science for the Humanities: What is Data
  • Steroid Data Scenario: Introducing Decision Intelligence

Similar Posts

Leave a Reply

Your email address will not be published. Required fields are marked *