When working on a feature we don't think in terms of the files that need changing, line by line. We think at a higher level, more conceptually. On a typical MVC web application, a route is related to a specific controller's action, which in turn is related to some model performing the business logic. We think of all of these moving parts as a single "thing", just different parts in the same assembly line.
Yet when we send a pull request and someone has to review our code, what they see is a series of changes. File after file. There's a whole layer of meaning that existed on our minds only, and now the reviewer has to reverse engineer it from the output of git diff. Instead of being able to see the data flow of an API call, our reviewer will see a bunch of controller files' modifications. Then maybe the models, and they have to go back and forth to understand what's going on. Then maybe at the end they'll see the new routes.
The exact details are highly dependent on the project and on our design pattern of choice but whatever the case, looking at a series of modified files, line by line, doesn't convey the same ideas and thought-process that was going through our heads while coding. The underlying structure and design is not immediately obvious by looking at the changes alone. And while the code is the final form of those ideas, it's the design, the solution that we came up with at a conceptual level, what makes the real output of our work.
Based on that observation, I think as contributors it's important to find ways to transmit that thought process to our reviewers. And while I doubt there's a perfect way to achieve that, there are various techniques we can use to make our work easier to understand.
The first one, which is arguably both the best and the hardest to achieve, is to keep the scope of pull requests as small as possible. Smaller pull requests, without lots of moving parts, are easier to understand by looking at the code alone. If they have a clearly scoped goal, each modified line can be clearly traced back to that goal. Smaller changes can be more thoroughly reviewed. When the reviewer can understand the whole scope of the changes at once it makes it harder for bugs to slip in.
That being said, sometimes this isn't achievable. It could be that a big feature is full of moving parts and it's hard to scope them to a single change that makes sense to merge on its own. Or it could be that we send a pull request for a refactor which necessarily makes changes across the whole codebase.
For that purpose, I think writing a good description is crucial for the reviewer's understanding of our pull request. The modifications are just a list of all the moving parts that we needed to achieve our goal, the description tells the story of our work.
So, what makes a good description? This is highly subjective, but I think a good description should strive for a balance between clarity and brevity. A good description is the shortest description that can get the point across to our reviewer. And that means that it not only depends on the changes themselves, it also depends on the reviewer! A traditional feature in an MVC app might require little clarification if it's going to be reviewed by senior developers. A link to the corresponding user story in your issue tracker for background, maybe some clarification if you decided to add a new dependency, but little more is probably needed. If, instead, the pull request is going to be reviewed by someone less experienced, or someone from another team who might have less background on the codebase, then a more detailed explanation can be useful to guide them through the changes.
Even though this is subjective I've found that pointing reviewers to the interface of the changes, so to speak, can be incredibly useful. Were you working on UI changes? A screenshot or a GIF are worth a thousand words. Is it an API endpoint? Point your reviewers to the tests that use that new endpoint. Fixing a bug? Again, the tests that trigger the now fixed bug are a good place to make your reviewers go to.
Think about it this way: imagine you were sitting right next to your reviewer, explaining your changes. If at some point you would jump to the app and show something visually, that's the moment to add a screenshot. If you were to show how an endpoint works, that's the moment to point to a test. Give your reviewers something to get started with so that they don't have to navigate the changes blindly.
All in all, a reviewer has to understand what our goal is, how are we modeling the solution, and how did we implement it. Looking at the changes alone can explain the last part. In many cases, pointing to a ticket on an issue tracker can be enough to explain the goal. And it's left for the pull request description to tie those together by explaining the thought process that led to our particular implementation.
Bonus
Not all reviewers will necessarily be developers. In our team, we've found it incredibly useful to include project managers and designers in the review process. Given that they don't necessarily have an environment set up to run our changes locally, deploying each pull request to a separate environment simplifies enormously the process of reviewing changes from a functional point of view. This might not make sense on every project but I highly recommend it for web applications wherever possible.
Cover picture by Irvan Smith, found at unsplash
Oldest comments (0)