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:   - a code review of a change to roslyn  - 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:


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!

Code versus Configuration

At Ethos we are building a distributed mortgage origination system and in mortgage there is a lot of different user types with processes that vary depending on geography.  One of our ongoing discussions is about how much of the logic resides in code vs. being in a workflow system or configuration.  After researching this topic for a bit, I have arrived at a conclusion that the logic should live outside of code very infrequently, which might come as a surprise to a lot of enterprise software engineers. Costs of configuration files and workflow engines First thing that I assume is true is that having any logic outside of the code has costs associated with it.  Debugging highly configurable system involves not only getting the appropriate branch from source control, you also need to make sure that the right configuration values or the database.  In most cases this is harder for programmers to deal with.  In many FinTech companies where the production data is not made readily acce

Intuitive Programming - Comments

Comments are a topic of vibrant discussion.  Ever since programmers could leave some text in the program that was ignored by the machine the debate started: “what’s a good comment, what’s a bad comment, why comment?” There are endless instructions to programmers that say many of the following things: 1) Describe your function! 2) Don’t write in the comment what you wrote in the code. 3) Tell people why you are doing what you are doing. What I think has been missing from this discourse is the audience for comments and through those audiences there is intent.  The code is being read, skimmed or analyzed by people and tools.  So what are the audiences and reading modes? 1) Maintaining and enhancing the code 2) Skimming through the entire module or file to figure out what the overall structure is 3) Reviewing the test files to check out the test coverage and edge cases 4) Seeing the docstrings of functions while being in a separate file altogether 5) Reviewi