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!

Why you should take the software job in San Francisco (or not).

Silicon Valley is an iconic place for technology.  Many people say this is the place for the “best and the brightest.”. Apple, Google, Facebook, Salesforce, Twitter and other top companies draw a lot of talent form all over the world and the largest chunk of VC capital goes to companies in the Bay Area, so it seems like moving here is a no brainer!

The real situation is actually not that simple, I believe there are three scenarios where it makes sense, but in many cases living in the Bay yields disappointing results.  The cost of living, housing situation, homeless catastrophe make places like San Francisco a lot less appealing to a lot of people.  So in what situations does it make sense to move to SF?

Startup founder raising millions There are many places to be a startup founder, but if you are looking to raise capital the largest pool of VC money is in the Bay Area.  There is an established network, events and conferences which give founders an opportunity to pitch more people th…