Is it worth investing hours describing what a bunch of lines of code do? Who will benefit?
The description of a PR describes what the developers did, and the code describes how they did it.
Me, every time I find out an incomplete/partial PR description 😊
The problem: the hard Reviewer’s life
Compared to the Developers who wrote the code, the Reviewers could not know the context or miss crucial details. A detailed description would avoid the Reviewers trying to understand/infer what the code does; that’s the hardest part.
Even PRs that touch just a bunch of files could be hard to read because the Reviewers must:
Try to get/infer/find the background context the Developers had when they started designing the feature (and, sometimes, small details are hidden in synchronous and not tracked conversations).
Try to infer/guess the design process and decisions of the Developer, which then turned into code.
Try to infer the little, daily choices that driven the Developers to write precisely the code contained in the PR.
Try to follow the code to infer/understand how it works.
Check out the PR locally, launch it, and check if it works (and simulate the edge cases).
-
In the end, providing feedback, including:
- Comment and discuss the design decisions before getting into the code’s details.
- Point out code smells.
- Point out edge cases the Developers don’t manage through the code.
- Point out implementations that are not aligned with what the feature should do.
Step #6 is the most important one for the Developer, but the time-consuming steps from #1 to #5 drain out the Reviewers’ energies.
More: the steps from #1 to #5 are useless! The Developer that opened the PR already knew the context but did not share it through the description!
Last but not least, the Reviewers cannot infer what is not in the code. They cannot imagine why the Developers discarded some solutions, if and why a piece of code is temporary (ex. waiting for some answers from the Designers), etc… Hence the Reviewers cannot validate the non-written things.
In the end, reviewing the code without context is time-consuming and deprives the Developers of receiving a thorough review (and, from a company perspective, it costs a lot, since Engineers have high salaries and their time cost).
The Solution: a detailed description
How to avoid the above situation? By writing a detailed description that could include:
Videos: a recording of the Developer’s screen going through the implemented features and the edge cases
Images: screenshots of the original graphic layouts and the end, implemented, result
Code Tours: journeys that follow the code and the data flow to that ease comprehension
Snippets and design/code decisions: the Developers explaining what and why they coded the different parts
Request for Comments: the Developers could have some doubts; explicating them allow the Reviewers to provide insightful feedback
Hints: Steps about how to programmatically test some hard-to-replicate edge cases, if any
Notes: every other relevant information that could help the Reviewers
Feedback: How did the experience of writing the code go? How’re the Developers feeling? Was it hard? Was it easy? Why? Etc.
With the above, the Reviewers can concentrate just on step #6 of the list mentioned above.
The advantages for the Developer
Writing such an extensive description saves an hour for every Reviewer but costs an hour to the Developers who created the PR, is it worth investing such time? Absolutely! The main reasons are:
Carefully thinking about the code: while writing the code, it’s easy to enter a sort of tunnel mode where many important details are left behind, and the code is way more complicated than it should be. Describing the feature and the code forces the Developers to re-read and re-check it again. Watching the code from another perspective (the tech writer’s eyes instead of the creator’s eyes) helps to have a less subjective idea of the code itself, resulting in fewer bugs and frequent post-mortem re-writing for the better.
If it’s too long to explain, maybe it should be split: PRs with large contexts are longer to explain and harder to be reviewed. Reviewers cannot guarantee the same detailed level of feedback when reviewing 100-file PRs; 5-to-15 files with the same context is a good size for a PR. The process of dissecting some commits, fully re-creating a branch from scratch, etc., has the same effect as the previous point.
If you cannot explain it, you didn’t get it: there is a massive difference between thinking to know something and to be able to explain it. The latter is a measurement of your comprehension. It could be frustrating, but we are more and more requested to explain things during our Development career instead of doing them.
Help the future Readers: other Developers, or you in six months, will greatly benefit when they will git-blame code to understand not what, but why something has been coded that way.
Is it easy?
No, writing such a description could be challenging at the beginning. If the Developers are stuck and would like to receive feedback before opening the PR, please reach out to other Senior Engineers 😊. Practice gets better.
After many opened PRs, and after reviewing other Developer’ PRs, the process becomes more manageable.
Conclusion
The last thought: I care so much about explaining what the code does because there are no right or wrong implementations, but the non-explained ones fall in limbo between the two extremes, usually closed to the bad extreme.
Related articles of mine
- A Case History: Analysing Hasura Console's code review process
- Improving Hasura's Internal PR Review process
- Re-building a branch and telling a story to ease the Code Review
Top comments (0)