Subscribe to this blog

Follow by Email

Search This Blog

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?

Comments

Popular posts from this blog

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!

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 accessible…

About 20 years ago when I started working in technology companies I remember “the best” engineers had similar patterns:
-They worked crazy hours
-They knew the systems no one else knew
-They could react and deliver something faster than anyone else
You could always hear other employees say: “Bob is really smart, no one knows how to get anything done in system X besides him!”

This reinforced optimization around being the only person who knew how to do something in some part of the code. That in turn reinforced job security and bargaining for those engineers, but also chained them to a particular system. We had big code bases of C++ or Java code where some “Bob” hacked up features as soon as he possibly could. “Bob” would have occasional nuclear disasters where he’d sleep in the office or through the weekend and then everyone would thank him for how he “saved the day.” “Bob” sacrificed his quality of life to get praise when he hacked stuff up quickly and then the second time when n…