Skip to main content

Pull Requests and Code Reviews

Software development involves a great deal of collaboration.  One of the most basic blocks of collaboration on a software development team is a code review.  There have been many different ways of doing code reviews over time, some of this has been dictated by the tools available.  Git and online source collaboration tools created a set of best practices that are worthwhile of adopting on any team.

About a month ago I have looked at various articles about how to best create a Pull Request (PR) and do a code review and the attached presentation is the result of this research.  The presentation can help you guide your team and develop a set of collaboration practices that works for your particular situation.

It’s good to start out with why to seek a code review.  Having clarity about your intentions helps you guide the person helping you with code reviews and also to manage your expectations about you can get out of the code review.  The reasons for seeking a code review are generally one of the following:
1. Advice from your teammates
2. A way to discover parts of the system you haven’t considered
3. A quick test for readability of the new code or changed code
4. Improvements comments and documentation
5. Insurance that there is someone besides yourself who understands how the code works
One thing to keep in mind is that the reviewer generally does not get the credit for the feature being done, so it’s always best to be very humble and thankful that they are spending time giving you feedback.

When writing a PR, given that you are using a system that supports it (like GitHub, BitBucket and Visual Studio Online), some good practices are:
1. Anyone could be reading this PR (including your executives)
2. Add context, the best feedback might come from someone who is not familiar with this code.
3. Ask for the kind of feedback that you want: quick looks, opinion, guidance, help?
4. If you are hoping certain people take a look @mention them.
5. Make sure that the code builds, that unit tests and basic tests are covered.

When you get the feedback first and foremost you should be thankful that someone took the time to review the code.  Reviewing the code is often a lot less fun than writing it and solving the problem.  Here are some things to keep in mind:
1. Avoid taking things personally.
2. You don’t have to make the suggested changes, but it’s respectful to say why.
3. It’s OK to create a ticket to address parts of the feedback later.
4. Some of the questions in the pull requests might be good comments to add to the code.

One the flip side of putting together a PR, there is also the job of a code reviewer.  It’s always great to make sure you know what you are getting out of the review and what the team gets out of the review.  Key reasons to conduct a code review are:
1. Learning about other parts of the system.
2. Learning about techniques, libraries and tools that you don’t know about.
3. Insurance that unknown or bad code doesn’t bet into the system.
4. A checkpoint for what is going into the code base.

Tone is very critical in a good code review.  Given that a lot of times today the code review is not done side by side we need to keep in mind that online collaboration system is limited medium.  Here are some basic tone tips that you should keep in mind:
1. Ask questions instead of making demands.
2. Accept that there are multiple ways to solve the problem.
3. We *all* own the code that we work on.
4. ”Always”, “Never”, “Nothing”, “Who would ever” – are all quick ways of sounding like an idiot
5. Talk about the code and the work instead of the person

A good and thorough code review is very valuable.  Providing good feedback is hard and takes a lot more time that people usually devote to it.  Here is a checklist that you can go through to make sure you are indeed reviewing the code and helping the team improve the code base:

1. Do you understand why we are making this change?
2. Is it the right magnitude of change (are we doing a hot patch and this is a major refactor)
3. Do you believe the kinds of tradeoffs that were made to implement it?
4. Does this change affect other parts of the system?
5. Should tests be added?
6. Does the change conform with existing styles and methods?

I picked out a couple of good examples of collaboration, feedback and descriptions:
 https://github.com/dotnet/roslyn/pull/24070   - a code review of a change to roslyn
 https://github.com/facebook/react/pull/11804  - a refactor in React.js


This research was done based on a lot of internet material but the key sources the following blog posts, thank you authors for sharing your insights:
 https://github.com/blog/1943-how-to-write-the-perfect-pull-request
 https://www.atlassian.com/blog/git/written-unwritten-guide-pull-requests
 https://github.com/thoughtbot/guides/tree/master/code-review
 https://blog.alphasmanifesto.com/2016/07/11/how-to-create-a-good-pull-request/

Post a Comment

Popular posts from this blog

SDET / QA Engineer Interview Checklist

After interviewing and hiring hundreds of engineers over the past 12+  years I have come up with a few checklists.  I wanted to share one of those with you so you could conduct comprehensive interviews of QA Engineers for your team.

I use this checklist when I review incoming resumes and during the interview.  It keeps me from missing areas that ensure a good team and technology fit.  I hope you make good use of them.  If you think there are good questions or topics that I have missed - get in touch with me!


SDE/T or QA Engineer interview checklist from Mike Borozdin
If you like this checklist you might want to check out these posts:
Emotional Intelligence in Software Teams  and Good-bye manual tester, hello crowdsourcing!

Two Critical Questions for Your Next Interview

I’ve interviewed probably over 500 engineering and management candidates over the last several years.  There have been a lot of really smart people who have applied at DocuSign, Microsoft and Tempo Automation. A surprising number of them didn’t have a clear answer to these two essential questions:

Why are you interested in joining our team?Why should we be interested in you? 
If you are an applicant, having a prepared answer for these questions is critical.  If you are a hiring manager, you should ask them and have a clear answer to these questions at the end of the first interaction with your future team mate.

In a field where work is somewhat predictable and static, those questions are less critical, but in software development perseverance, ingenuity and focus make all the difference. These are the two main questions that will separate a subpar and a superb hire.

When I discuss those two questions with an applicant I try to go below the surface.  Generic answers like “it says you ar…

Ego in Your Decision Making

Oxford dictionary defines Ego as: “a person's sense of self-esteem or self-importance”. It says that it is “responsible for reality testing and a sense of personal identity.” For a lot of us techies those were really important functions that probably served us very well early in our career.  We had to be assertive, we had to choose a coding style guide, pick our frameworks and “be decisive”.  However when it comes to engineering management the same things that used to help might be turning into a liability.

When my job changed from writing code to creating an environment where the best code is written - it took me a while to understand how my ego was holding me back.  I kept applying the old tricks - defining the architecture, the final design and task priority.  What ended up happening is those things led me to turn off a lot of the best and brightest people on my early teams.

I remember the feelings of being aggravated when I had to go over an explain my decisions and get buy …