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!

Chief Collaboration Officer

When you search for the word “collaboration” on the Internet, the top hits are mostly software packages you can buy.  Software can facilitate collaboration, but it doesn’t make people collaborate on its own.

One of the key functions of a technical leader is to bring a team together, help people share ideas, and facilitate team members helping each other.  When a software leader overlooks this key function, you end up with a group of individual contributing engineers instead of a cohesive team.
Before we get into tactics, we should ask “Why is collaboration important for an engineering team?” 
It’s critical to examine your assumptions, so here are my reasons for why a group of engineers working on their own are worse than a team working together: Smart people learn from each other.Getting your plans and designs reviewed by other people allows you to leverage their experience and check your assumptions.Collaboration produces artifacts that stay after collaboration has taken place (such…

Hire Fast, Fire Fast? Not so Fast.

Silicon Valley is full of advice and it frequently comes from people who have little experience on the subject matter.  A popular topic surrounds hiring and terminations with the king catch phrase being: “Hire Fast, Fire Fast.”  To me, what that usually means is lack of diligence, thought, communication and courage.

When hiring people love going with their gut feel, often with disastrous results.  There is an obvious subject of diversity of thought, appearance and background.  When thinking “fast” you are probably hiring people like yourself because humans quickly react to people who they believe are in their tribe.

A startup that lacks the resources of a big company often becomes so desperate to get technical staff that when a decent candidate comes along, excitement ensues and the employer doesn't slow down to put them through a more rigorous hiring process.

I highly encourage technical founders and engineering executives to write out their precise hiring process.  Of course, y…