Decoding Code Review, Webinar with Elaine Watanabe

The following text is a translation of the transcription of Elaine’s presentation Decoding Code Review. The Webinar was recorded on March 27th, 2020. Some not essential parts were cut off. References are found in the video.

Decoding Code Review

George: Let me introduce quickly, Elaine will calmly introduce herself afterward. But this is a super cool content. I’m a fan of this content, I’ve watched it several times, I’ve watched a lot of presentations by Elaine, and I really like the way she synthesizes a lot of things, cool, so I always recommend actually always sending her slides, PDF, I was super happy to do this Webinar with us.

Elaine worked with me at Plataformatec for a while, I did her hiring process, I interviewed Elaine and now she is as a developer at TheRealReal. Elaine, feel free and I’ll turn off my camera and leave you with the folks.

Elaine: Nice. Thank you, George. I worked for two years at Plataformatec, and this year had great news. I’ll start here.

This is a topic that I really like to talk about, Code Review, it was one of the topics that we always addressed in the onboardings of the folks from Plataformatec’s engineering team.

I like to put this photo to talk about how, if you remember how you, for whom is a dev, in the technology area, at what moment did this area speak to me? I like to put this picture of my childhood so I can remember everything, where I was, where I arrived, what were my references, where I went.

I am currently a software developer at TheRealReal, I have a master’s degree in Computer Science, I am graduated from USP, at the Institute of Mathematics and Statistics. My slides are already available, I need to update with this version, but I already have the PDFs there, and this is my little entrepreneurial photo, in my first event as a mentor in Campo Grande.

This lecture has already given in many places, has already been presented in Agile Trends, The DevConf (TDC), Guru, Ada Lovelace in Curitiba, I thank Professor Rodolfo for the invitation, and at RubyConf on the Codamos trail.

And what is the idea of this lecture, presentation, is to discuss a little of the challenges and practices of code review. Starting with the definition: what is Code Review?

A process that we check the system through the analysis of the source code. And it needs to be done by humans and basically you open the source code which is a text and analyze it and see if you understand it or not.

And what is the objective? Identify defect. And if we look at the processes that most reduce defects in our day-to-day lives, we can look and know that with code review, we can identify about 60% of these defects, and we start to think, what are the types of defects, what are the types of bugs that this process it catches, normally we already have a QA process as well, which identifies a good part of the defects, of the functionality errors.

The thing that surprised me most when I looked at this paper was that code review is a great tool to identify defects related to the Code’s evolution, which are made that are not identifiable in the testing phase.

What is evolution? Is it the ability to adapt your code, your system, over time and how does it evolve, how does it incorporate new features, how does it update existing features, and how is this evolution normally?

Can we always evolve smoothly, can we always add new features in an easy way?

Thinking like that, I already have ten years as a programmer, in all the career moments I had, in all types of companies, I always had this feeling that they were “ifs” everywhere, at least at some point, in any choice.

It may be the company that is starting or it may be a more stable company, at some point, I had this feeling “oh I have to put an if here”, and it is part of it.

We always need to think about these types of decisions on impact over time. If we look at the concept of technical debt, which is when we, we choose, we make a decision that we know that we will have to change in the future, for example, “oh, I’m going for an if here” and ahead I change to a model of classes and subclasses and things like that, you get a debt, something that you will have to pay in the future.

And these decisions accumulate over time. At some point, you may feel that everything is on fire, that it is full of bug tickets, but it’s okay, and it’s really okay, it’s part, but we should remember that bad things can happen if we never look for that debt.

We can arrive at a time when we make these quick decisions to add value to the business quickly, but suddenly we find ourselves in a scenario like this where the amount of bugs is so high that we are opening more tickets to correct errors that already exist than adding new features.

The Code Review, the code review, can be a tool, a way to help reduce this type of debt, this type of problem. An interesting part, depending on where you incorporate this process, it usually has a lower cost in relation to the correction of defects in production.

For example, between development and testing, the cost will be lower, considering the number of people and time, than for example at a cost, the cost of changing something that is already in production, it had the entire development stage and would have to go back to the stage, was from development to production and would have to go back to the development stage, would have all this accumulated cost.

Thinking… Returning to the issue of Code Review, at the cost of, the issue of having a lower cost, it can also have a positive impact on the quality of the software. The software as a whole, a system as a whole, when we talk about quality, we are thinking about reliable systems, thinking about systems that work correctly, are efficient, have performance, are easy to maintain, maintainability, that adds business value, it is useless to be well done and have no return for the final user or the business.

Speaking of Code Review, thinking about the long-term impact is not something immediate. Sometimes you’ll only see the benefits over time. So we defined, we understood why, how is this practice in everyday life normally?

How do we do a Code Review? There are good practices that I’ve worked with, full-time pair programming, I would spend eight hours with another dev together with me, coding, two keyboards connected to the same machine, two monitors to have a minimum of ergonomics, but it demanded that you have to work at the same time in very great harmony and sometimes the harmony was so great that even the color of the clothes we combined unintentionally, they were not trivial colors, I am in gray and so is he. Maybe I was wearing pink and the person was wearing pink too, at a very curious level of attunement.

And we had a hard time tracking decisions over time too. It was in the context of a startup. We couldn’t quite understand why, after a while, why a feature had been added. We had a lot of dev turnover as well. And then I decided to focus, in this presentation, on the concept of Pull Requests, which has a face of, Pull Request or Merge Request, depending on the tool you are using, and that has this face here.

It is a document, which informs you of a change, it has a title, a description, you ask people, ask people to review it. And it is done by means of comments, so the interaction goes from being, for example, verbal to be by text. And it’s through…

It’s a conversation. This tool is cool, it allows you to compare the changes from the original code with the proposed change, you can see the diff, see lines added, changed, removed.

But why? Why, why do we use Code Review, what benefits does it bring?

One thing that marks a lot when we have a well-done Pull Request context is that we can link the code change, the transformation, with the business context, why the code was inserted there. And we can also access the history of the discussions, for example why this decision was made in the past, we identified a technical debt that we decided to assume because there was a matter of risk, a matter of deadline, and reasons, and we can see this type of discussion that took place in pull requests, mainly in people who rotated from teams and things like that.

A really cool talk about this versioning issue is this one, from RubyConf, A Branch in Time. It brings the idea of capturing why and not what is being done. “Oh I’m changing the user”, no, “because I need to add a new type of sign in”, for example.

And a very interesting part of these documents is that it allows for asynchronous and distributed interaction. You don’t have to be interacting with the people on your team at the same time. You can be in different places.

Thinking about our current situation, in terms of remote work, the matter of people working in different time zones, in different schedules, in different places, it is a great tool that adds value to this type of interaction. Please stay home if possible.

There is another side, too, that we end up not looking at, which is the transfer of knowledge, we are able to exercise this mentoring role among the dev teams.

We can also get a more senior person to interact with a more junior person or even a newly hired person, who doesn’t know the architecture standards, doesn’t know the kind of decisions, you can pass this type of information through Pull Requests.

It can also give visibility to changes to other teams. For example, you have several squads, each one has several people opening multiple Pull Request in the same repository. You can communicate these changes to other people.

Thinking a little more… We understand the why, going to the side of good practices, then I like to divide it into three categories: as an author, as a reviewer, and as an organization.

The author of Pull Request

It is very important to put an explanatory title, to ask the matter of motivation, the context of the business. We can also put a list of questions and previous discussions, maybe we are going to do it that way because we had a discussion about architecture, there is already planning that is not visible in that change.

It’s important to inform the rest of the team. The cool part is also putting gifs, screenshots of the changes, especially when we are dealing with the frontend or mobile app.

It is also important to have consistent commits messages. It is useless to be informing one thing and doing another, maybe the person is joining a giant refactoring with a change in value within the business context. It is also important to place the complete and tested code and always aim for small changes.

I think everyone has seen a little gif, an image speaking I have five lines of code, lots of comments, I have a hundred lines of code, LGTM.

If we want to add value with these changes, with this process, it’s good to focus on this type of recommendation. And a very nice recommendation, stolen from the principles of SOLID, Single Responsibility Principle, not to mix a big refactoring with a feature with a bug fix, that one thing can stop the other and it will also make it difficult to review.

Sometimes the bug fix is right, but the refactoring will have a huge impact, you block your ticket, your change, because of that, right?

What else can we do? One thing I saw yesterday talking about people who have difficulty opening a Pull Request because they still don’t feel so safe. You can follow all the recommendations, make a checklist, it’s consistent, informing the value of the business, it’s making sense, and adopting the practice of revising your PR, your Pull Request.

You can add your own comments, here I’m not comfortable with our decision, any suggestion?

Remember to tag people, ask for a review, we can later enter into a discussion about who I call to review.

Remember to apply the necessary changes, according to the comments, always remember to respond to everyone’s comments. Like it or not, the person invested some time to review it and I think a return is important. And sometimes there is information and it can bring you very important insight.

A tip is also to use the Pull Request templates, we can do it, every time we open a Pull Request, you can have a minimum need. I want a description, a motivation, how to test, what kind of change is being made. There are a few ways to do this.

As a proofreader or Pull Request reviewer, what do I need to do?

I need to look if I can identify any kind of defect, some kind of bug, whether the logic is implemented wrong, or maybe there is something else. We can also suggest alternative solutions, suggest refactorings, reinforce standards of code and design, the company maybe has a pattern to follow.

We also have to validate the functionality, if maybe what business is waiting for is being reflected in the code.

Sometimes the person does not know the whole and ends up thinking that the change he made is restricted, you can validate that kind of thing too.

You can identify security issues, maybe there is a string there, an SQL Injection, you can analyze the performance issues, the n + 1 of the ActiveRecord.

You can suggest documentation, remembering that documentation is not commenting, they are different things, telling you how to use the system, why that is being changed. It’s another thing to just say what the system is doing, and remember to validate the quality of the source code, to look at the matter, for example, if the code is readable if it is being understood if it somehow an improvement can be made.

Pull Request review is a great way to get to know new features, to get to know mainly on very large systems, to learn new technologies. I have learned a lot by reviewing pull requests for iOS and Android apps, and consulting the previous changes has also helped a lot in this learning process and I take the opportunity to share knowledge and questions.

Maybe you know something that the person does not know, or maybe you are curious, that even in my case, who is learning a new language, I comment on pull requests asking questions, about things that are part of my language learning process, not necessarily related to the change.

Free E-book: Click here to download the free e-book Decoding Code Review and Pull Requests. Get all the key points and best practices to implement a code review culture.

And what are the challenges?

It seems to be a very simple, very trivial process, how we pass this baton, how we exchange this information. Sometimes we need to use this gif not always just having the process is a way for you to get reviews, you can give visibility to the change, you can make the flow to flow, “hi, review my PR, please”.

How do we give visibility? We can have notification tools for Pull Requests, for example, I use a tool integrated to Slack.

We can have metrics to monitor the project, for those who use Kanban, we can have metrics related to this. SourceLevel I know that there are some metrics that George can present later, and it is a way to raise awareness, we can use the matter of bug tracking tools, whether the quality is rising or falling.

People say “oh, I’m going to add another process, another stage in the development process, it will impact the delivery time, right?”. I did some research on that.

I forgot to mention it, but I really like to relate my academic life, the matter of scientific articles, with my day to day development. So there is this mix of articles by academics with my day to day development.

And looking at this article from Google describing the Code Review flow there, informs that 70%of the changes in the Google are integrated in less than 24 hours after the request for review.

So imagine the impact of changes to the Google system, how much of a wrong change, how critical it must be to make that kind of change, it was quite shocking for me to know that are integrated in less than 24 hours.

And there are some important tips on this process. In the case of small changes, it is necessary for only one person to review, neither have comments or just an authorization for integration, and everything is fine.

It seemed to be a bit of magic, but looking very closely, it is actually alignment, but if we can have a very integrated team, we can have a codebase that will appear as if a single person had written it, and this Patrick talk has a very interesting discussion about it if your codebase looks like it was written by a single person.

I already came to know, for example, a random variable in the test who had written that code or not. Very simple things like that.One thing that is important to encourage in this process is to understand, to encourage collective code ownership that everyone is responsible for by the code.

It’s not just the person who is making the change, and whoever reviews is also part of the solution. The code belongs to everyone. And how can we go in that direction?

Every person, regardless of role, regardless if I’m reviewing or creating a Pull Request, I can, I need to remember that the feedback needs to be about the code and not about people, that there is an error, there is an error in the code and it doesn’t mean that the person,

It doesn’t mean that I need to judge a person based on their code. Nobody wakes up and says I’m going to add a bug and I’ll be right back. We are there to develop features and solve problems we will not always find the best solution. Why not find it together?

One way to encourage it is the participation of everyone on your team. It is not because someone is experienced that they will not go wrong. I’ve heard people saying that person is super senior, they don’t make mistakes, so I will approve the Pull Request.

It’s not because you are a beginner that you will not have a contribution. Maybe a question. I didn’t understand this flow here, could you explain it to me? To show how much that code is not readable, it is not clear. And there may be some refactoring that will help maintainability in the future.

Another way to go in this direction, it is to adopt the use of explicit and descriptive comments. For example, the person commented here only with emojis, with single scissors, and the first time I saw this, I was like Is it to throw away the change?

What did I wrong? In the end, it was just to erase an extra space, they could have put “remove that extra space, please.”

Repetitive comments about code style, you can easily substitute for code analysis tools, let’s use the bots to our advantage. You don’t have to comment, “oh there’s extra space here”, “extra space here”, “extra space here.”

We should use the tools to our advantage. I’ve used it, I use Robocop a lot, I come from a ruby context, and SourceLevel, I’m not at Plataformatec anymore, but I’m still advertising.

It’s a bot that allows you, SourceLevel has a bot that allows you to comment on styles, you can define the rules and you no longer need to be afraid of being the boring and inconvenient person commenting about it. You reinforce this in a more organic, institutionalized way.

Also remember that when you are going to suggest a design improvement, we can suggest and deliver in another Pull Request, you don’t have to block a change just because we think that code needs to be improved urgently.

Being able to separate what is delivering value to the business, what I am delivering of value to the system, and also remember that if it is difficult to reach a conclusion, sometimes there are rains of comments, rains of interactions, remember not to be limited to the review tool.

We have other tools such as video conferencing and we can talk in person too. But please, only after the quarantine.

But don’t forget to comment on the decisions and discussions that were made offline within Pull Request Elaine: to keep the history and update other people who did not participate in this discussion is also very important and the part that I think is valid for other types of interaction, paying attention to the way you communicate.

It is not always, it is not always obvious that comment or behavior is harmful. Don’t be afraid to ask for feedback, ask for feedback, give this opening to other people, saying oh, here I felt a little uncomfortable or not and take feedbacks for that too.

The organization

Wearing the organization hat, from us, as a company, what can be done, very simple things already help in this interaction. Define a minimum number of criteria for approval.

Maybe, ah, there are ten people reviewing, the minimum is two people and you can send it to production, already reduces the number of blocks, bottlenecks, in your process, and don’t forget to look at the social factor, this study is very interesting, it’s a study based on GitHub, but I think, I saw a lot correlation with my day to day development.

When we review the Pull Request, for it to be approved there is a technical factor. If you have the tests along with the change, it is 17% more likely to be accepted.

But if the author follows the person responsible for the project, they do not necessarily have a strong bond there, there is 187% more chances of PR being accepted, there is a social factor, some people define it as merge buddies,
my merge friends. You approve of mine and I approve of yours, and life goes on.

You have to be aware of that as an organization, it is recommended to formalize recommendations, creating guidelines, including on behavioral aspects. We are talking about teams, we are talking about people, we are talking about verbal, non-verbal and written communication.

In this part, I get to a point that is not so obvious, in the case of holes without signpost, things that we do not see, which are toxic behaviors.

What are these toxic behaviors? These are behaviors that prevent innovations and ideas, which we have always done so, we will not change, they promote a culture of non-communication.

I’m not going to talk to that person because they’re going to scold me, they’re going to be rude, they’re going to say I’m wrong. There is a point, it usually happens because there is a person who centralizes this information.Only that person, only that dev knows, that dev person knows that part of the system and that ends up putting the project and business at risk.

It is a centralization of information. If something happens to that person, nobody else will be able to understand that system.

It also has a characteristic of aggression, verbal, non-verbal, written aggression. Then I brought some examples.

• “How come you don’t know that?” I already heard that. It was a joke, but…
• How did they let you in here?”
• “Will I have to explain again?”

This can then be in writing, verbally, right. But what does this type of interaction add, right?

It has, it has a negative tone. If we look at the analysis of feelings in comments, mentioned in that study from Google, we have evidence that comments with negative tones tend to be less useful, what is adding me to the information?

It’s actually blocking me from communicating and interacting. How can I avoid this?

We adopt something simple, like making reviews as human beings. I think this post is wonderful, with examples.

Then I have an inversion, a pun, the “ask, don’t tell”. It’s just to ask instead of saying do that, no, could you do this? Something like this.

Then I’ll show you the examples, just change the tone, “aren’t tests important to you?”

Here comes the question in a sarcastic tone, and sometimes as the person just forgot to add that Pull Request. No, it is not recommended, or Request Changes if it comes “this PR cannot be merged.”

OK, but is it an opinion? What concrete actions do I need to take to make it mergeable? Either I accept it or I don’t accept it, and life goes on. It is not a useful tone.

Why didn’t you create a new class? We can ask in a very automatic way, but we don’t realize that it has a personal judgment tone. like how did you not think of that?

There are other ways of us to improve this interaction, seek to comment in a more constructive way. For example, in case you suggest a new class, you don’t always have a context, so suggest, what do you think about extracting this logic for a new class? And justify, hows that it is not just an opinion.

I believe it will improve readability and reduce complexity. Make it clear that you do not have it, that you may not have the entire context of that process, remembering that there is maybe a decision that was not documented, it was a technical debt that will be contracted, which will be resolved in the next Sprint, for example.

“I don’t know if you have already analyzed this but is it not worth creating a new class for this case?” It was a more suggestive tone, without supposing anything.

And what are my learnings about all this, mainly living for two years in consulting?

Software development has a lot to do with culture and communication, even if we don’t look at it in depth. And when we start looking at culture, what Chimamanda says is that culture doesn’t make people, people make culture.

We do not enter and have a ready culture and we just accept and go. We are transforming it every day, we are creating, we have our part in this type of interaction too. We are responsible, like the Pull Request case, when we review, we’re part of the creation too.

One way of looking, like this communication, like this culture, is for you to look at your team, what is the interaction like, what is the diversity of this team.

In my experience, I see that diversity helps stimulate empathy and help and can help too to reduce toxic behaviors. Sometimes, we live in our bubble, we don’t realize that we are reproducing toxic behaviors. Sometimes different people help to bring that kind of vision. And it can positively impact innovation and profit.

When we don’t just have a closed group of similar people, we get different ideas, we get to expand the horizons of our system and impact innovation and profit. This very cool study by McKinsey that talks about the issue of gender diversity, that when we have gender diversity in leadership positions, we have 21% more chances of results above the market average.

When we talk about cultural and ethnic diversity in leadership positions, we can achieve up to 30% above the average. We also have to look not only at people, our team, but we also have to look at the environment, for non-technical factors, pressure issues, overload of activities and business context, which totally influence the quality of the software and the daily routine, and the code review process as well.

The system, the software, will be a reflection of all these factors. It is not just the final software, the written code. We forget that when we write a code we are communicating. It is a way of communicating. I like to reflect on this, this quote from Donald Knuth, the father of algorithm analysis, it brings this whole context of mathematical analysis applied to the computing context, tests.

And he has this quote, this very cool line of reasoning talking about the matter of imagining creating one, going to the communication side, right.

instead of imagining that our task is to instruct a computer what to do, let us concentrate rather on explaining to humans beings what we want a computer to do.

Donald Knuth

Let’s remember that we are talking to other human beings via computer. That’s why I say Code Review is about culture, about people, about software quality. And I want to know how the Code Review is in your daily life.

I will leave here some references that I used. There are these references from Microsoft, it’s from Microsoft, from Google, it has Google’s practices documented too, there are some things about refactoring, about code improvements.

Sandi Metz’s book is wonderful, especially for those who are object-oriented programmer, Uncle Bob’s books about Clean Code, agile processes, if we look at the communication side, I think these two are my favorites. Non-Violent Communication and Radical Candor, about people interaction and feedback, the matter of culture, the matter of bias, the Thinking, fast and slow brings this matter of what is unconscious bias about teams, about team dysfunctions. I think these two bring very cool approaches too.

I left a curious article for you to read about the Code Review process using a very cool MRI experiment, and a series of other links and videos and podcasts.

I love this tweet from Roberta, she works at StackOverflow and she brings this discussion, do we always need Code Review?

And it is a supergiant thread with several trade-offs about this process. Whenever I see, I reread that thread.

And that’s it, I think. As always the propaganda of my group, of the group that I love to participate, this year I’m in the organization, Rails Girls. Anyone who wants to talk about how to increase the diversity of, in teams of development, Rails Girl has a great Workshop on that.

You can call me on Twitter. And then a spoiler, I’ll be at Elixir Brasil giving an introduction workshop to Elixir. Please note that the date has been changed to November 28 and November 29.

Whoever wants to talk about it, I’m here. And that’s it, thank you very much!

[wd_hustle id=”9″ type=”embedded”/]