Recipe useful code review from a developer from Yandex



    Hey. My name is Sergey, the last five years I have been working at Yandex. During this time he participated in the development of eleven projects. Wrote code in javascript, python and c ++. Some projects were done alone, others were developed in a group of eight people. But in every team, on all projects, regardless of programming language, I used code review.


    With the help of code-review, I constantly learn something new. Sometimes, looking at someone else's code, I want to exclaim: "And what, is it also possible?" In someone else's code, I find interesting tricks and take them into service. Many new knowledge scoop from comments to my code. It was a discovery for me that people love to share their experiences. Even when I develop a project alone, I ask the guys from the other team to see my pullrequest. It motivates to write beautiful and clear code.


    But it was not always so. Once a review was a punishment for me. I could write the code for a week with inspiration, putting all my energy into it. I sent a pullrequest, pinged the reviewers three times, and in return received a dry “like ok” or, even worse, dozens of comments on the merits.


    I came to the review pools of five thousand lines. I spent hours trying to figure out the code, scrolled from function to test and back again a hundred times. He wrote dozens of useless comments about the missing semicolon. All this terribly annoyed me. I often put off reviews for later, and I had dozens of unwatched pools accumulating.


    If you felt it on yourself, then the article is for you. Today I will talk about the techniques and tools that I use every day for five years, a daily code review.


    "Before the review." Tips for the author


    Imagine that the solution to the problem is cooking. You work in a team, so you need not just to cook, but also to teach this to other chefs. It is not enough to show them the result, you need to write a recipe.


    Commits


    Each step of the recipe is a commit: two eggs were broken — commited, a glass of milk was added — commited, two hundred grams of flour were poured — again commited.


    In each commit I express one simple thought. This can be an implementation of a model method or a component in layout. So it will be easier for the reviewer to understand me. I do not blame it on the whole task, which can not be swallowed at a time, but I talk about the decision piece by piece.


    Any refactoring I put in a separate commit. Often, refactoring is purely technical in nature, for example, renaming a method. The viewer does not need to read every line of such a change. He runs his eyes "diagonally" and will be able to devote more time to the more important code.


    Crumble, break, chop your code into small commits. This will allow the reviewer to better understand your code. It's okay if you overdo it with decomposition. Two commits are easy to merge into one. It is much more difficult to divide a large commit into several small ones. "Chopped vegetables" is easy to get by mixing "chopped tomatoes" and "chopped onions." But to get all the ingredients from a plate of salad, you need to spend much more time.


    After the commit, I immediately push the changes to the githab. It rescued me a couple of times when there were "coffee troubles" with a laptop.


    Description of commits


    When I write an email, I fill in the title and contents of the letter. The heading is a short and capacious title, the body of the letter is a detailed description with pictures and links. I apply the same approach to the description of kommit.


    I have long refused to commit a team git commit -m 'fix1'. Instead, I execute a command git committhat opens a text editor. In the first line I specify a short and capacious description of the commit (as in the letter header). After the white space, I describe the implementation details (like the contents of an email).


    I do not like recipes where they write "add a pinch of salt" or "bake until done." Each has its own pinch, and looking at a chicken in foil I cannot determine its readiness. In the description of commits I try to get rid of all ambiguities. Using ASCII graphics, I describe a test case. If the decision was preceded by a discussion where we drew charts on a blackboard or in a notebook, then I attach a link to the description to the description.



    (example of a commit description using ASCII graphics)



    (an example of a description of a pullrequest with a heading and a body. To edit a comment, I used the vim console editor)



    (an example of a commit with a description on GitHub. Use the arrows in the upper right corner to move between commits)


    Self test


    Before serving, the cook takes a sample, puts it on a beautiful plate, adds decoration. We will do this with three teams:


    git status
    git diff comments.js
    git add comments.js

    I execute them as many times as the files have changed. Intentionally do not do git add .to view each file separately. So I check myself, I look at my code with a fresh look.


    Code Style Check


    I can not imagine my life without a linter. This is a tool that automatically checks the compliance of the code with the selected style. For javascript I use ESlint . You can compare the linter with the R2-D2 robot from Star Wars, which walks through the code and puts it in order. Places that he can not fix himself, underlines red.


    With my favorite WebStorm, this linter works out of the box. I see and correct the problem as soon as I wrote the code, and do not leave this work for later. Before the commit, the linter is started automatically with husky .


    The code after the linter is nice to watch. I save the reviewer from having to write dozens of useless comments about the missing space. All attention will be focused on really important things.



    (example of launching linter to commit)


    Pulrequest description


    If a commit is a recipe step, then a pullrequest is an entire recipe. When all steps are described well, then write a recipe is not difficult. You can create a description of the pull quest team git log --pretty='%h: %B' --first-parent --no-merges --reverse.



    (an example of the command `git log --pretty = '% h:% B' --first-parent --no-merges --reverse`)



    (example of a description of a pullrequest by text, if you copy the output result of the previous command)


    Comms hashes become links on which the viewer can navigate. They remain working, even if you combine all the commits into one. More about the merger talk further.


    Pulrequest is ready! But do not rush to close the task, the most interesting is yet to come.


    "During the review." Tips for the reviewer


    Let us briefly distract from the creation of a pullrequest and imagine ourselves in the place of a reviewer - the person who was sent the code for review.


    Architecture Review


    Before looking at the code, I try to understand what problem the author solves. I read the description of Pulrequest, I follow the links in ishju, I study the schemes and photos. Sometimes, the error lies in the formulation of the problem. A good reviewer assesses the idea, and does not act as an "advanced linter".


    In any incomprehensible situation I ask a question. Even if it seems stupid or banal to me. With this question I can push the author to the right thought.


    The task of the reviewer is to show alternatives.


    In some cases, I disagree with the proposed solution. This is normal! Each programmer has his own opinion. If the author's code is objectively good, then I am not trying to convince him. I make the most of the offer, backing it with links, benchmarks or pictures. This allows you to conduct a constructive dialogue, and not go to the individual.


    Uniform style


    The author's code may not look optimal at first glance. It is written as is customary in the project. In this case, it is better to leave the option of the author.


    Let's look at an example. The author wrote the summation function for an array of numbers in the following way:


    functionsum(arr){
      return arr.reduce(function(res, i){
        return res + i;
      }, 0);
    }
    sum([1, 2, 3]); // 6

    This code can be rewritten more compactly using the arrow functions:


    const sum = arr => arr.reduce((res, i) => res + i);

    But the project started when there were no switch functions. If you write a compact solution, then the code will cease to be uniform, it will become difficult to understand. Either we leave the author’s version, or we rewrite all the functions on the arrows. The main thing - to keep a single style.


    Offline


    Sometimes, a very large and complex pull-quest comes to my review. The author tried for two weeks, but I have to realize the flight of his thoughts and delve into each line in a few hours. One I can not cope.


    In this case, I ask the author about offline review. I put a folding chair next to it (by the way, do not use a soft one: you risk taking the review for a long time), soot the author next to you and ask him to tell him about the decision.


    The effectiveness of this approach is low. First, time is spent by two developers. Secondly, it is easy to start abusing this practice: the temptation is too great not to think independently about the author's code (let him come and tell). Thirdly, I immerse myself in the course of the author's thoughts, unwittingly taking him for the right one - so I do not develop my point of view.


    A dangerous practice, but sometimes you can not do without it. Use offline review with caution.


    "After the review." Tips for the author


    Let us return to the pull-quest, which I prepared in the first part of the article. The reviewer has written comments. There is a stage of revisions and discussions.


    Dialogue


    If I agree with the remark, then I correct it and write in the comment that I fixed it. The reviewer will be pleased to see that I listened to him, and I immediately see which comments are corrected, and which are not. If you do not agree, then I ask the reviewer questions: why does he think so? did he understand my point correctly? how can he reinforce his point of view? I try to establish a constructive dialogue, during which we will get to the bottom of the truth.


    I respond to comments in the files tab. So the reviewer will receive one letter with all notes and questions, and not one letter for each comment.



    (an example of a response to a review: comment on the file tab and thank the reviewer at the end)


    In the end, be sure to thank the reviewer. He spent his time digging into my code and making it better. Doesn't it deserve nice words? Next time this person will take up my pullrequest with more enthusiasm, because he sees the importance of his advice and respect on my part.


    Commit history


    Microcommit partitioning helps with code review. For the history of this separation is redundant. Before that, I tried to divide the code into commits so that each commit describes a recipe step. The time has come to combine the pulrequest kommites into one in order to get a ready-made dish that will be easy to operate on when setting the table.


    Combine commits can be a team git rebase --interactive master. Let me remind you that I work in the feature chain FEATURE-1, and masterthis is the name of the main branch. After the command opens a text editor where the first commit leave unchanged, while the second and subsequent replace pickon squash. So the contents of the second and subsequent commits are added to the first.



    (an example of a rebuy in which the last four commits merge into one)


    After that, you need to push the change with the flag --force. I recorded the history, and a conflict of the local version and the previously known one arose on the remote server. With the command, git push origin FEATURE-1 --forceI inform the server that the local version of the changes must be considered correct. Everything is ready to infuse change.


    You can circumvent the difficulties described above using the new features of the GitHub interface. To do this, before Merz choose the item "Squash and merge".



    (example of combining commits when merging through the GitHub interface)


    After cooking, the cook removes his workplace, washes knives and wipes the table. I will do the same with the branch FEATURE-1. I delete the local version with the commands:


    git checkout master
    git pull origin master
    git branch -D FEATURE-1


    (The server version of the branch can be deleted by clicking the button immediately after merge pullrequest.)


    Conclusion


    In conclusion, once again briefly about the commands that I use:


    # Создаю и переключаюсь на фичебранч
    git checkout -b FEATURE-1
    # Отсматриваю изменения
    git status
    git diff src/controllers/v1/comments.js
    git add src/controllers/v1/comments.js
    # Создаю и пушу коммит
    git commit
    git push origin FEATURE-1
    # Готовлю описание к пулреквесту
    git log --pretty='%h: %B' --first-parent --no-merges --reverse
    # Исправляю историю после ревью
    git rebase --interactive master
    git push origin FEATURE-1 --force
    # Удаляю фичебранч
    git checkout master
    git pull origin master
    git branch -D FEATURE-1

    Questions


    What to do if you need to make changes to the old commit, which I have already done?

    Две команды:


    # Добавляем файлы, которые нужно прикрепить к последнему коммиту
    git add comment.js
    # Прикрепляем к последнему коммиту
    git commit --amend

    Если правки нужно внести в более ранний коммит, то есть два варианта. Самый простой — сделать новый коммит и объединить его со старым. Вариант посложнее — выполняя команду git rebase --interactive master, нужно перенести строчку с новым коммитом под старый и заменить pick на squash.



    Если в рабочей директории нет изменений, то вы можете сразу выполнить git rebase --interactive master и заменить pick на edit возле того коммита, в который нужно внести исправления.


    I got carried away and wrote a lot of code, but, fortunately, I understand well how to break it into commits. What to do?

    Нет проблем, если разные файлы попадают в разные коммиты. Чуть сложнее становится, когда один файл нужно разбить на несколько коммитов. Для этого подходит частичное добавление. Сделать это можно командой git add --patch test/comment-test.js



    As you advised, I divided the whole code into small commits. This includes commits that have edits to other commits. How to tidy up?

    При выполнении команды git rebase --interactive master первым делом нужно определить порядок коммитов. Выстраивайте строки с коммитами в нужном вам порядке. Строки с ненужными коммитами удаляем. Если нужно объединить несколько коммитов, то у первого оставляем отметку pick, остальным заменяем pick на squash.


    How to attach a link to a photo?

    Копируем картинку в буфер обмена и вставляем в любое поле ввода на гитхабе. В комментарии к issue, коммиту или в поле описания пулреквеста. Спустя несколько секунд получим ссылку на картинку.


    How to make a high-quality photo of a marker board or drawing in a notebook?

    Я установил на сотовый телефон программу Office Lens. Она вычищает лишний мусор с фотографии, кропает, убирает искривления и позволяет быстро пошарить картинку в любой чатик.


    Is humor appropriate in the comments?

    Если по делу и без злоупотребления. В общем, всё как в жизни.


    Also popular now: