In the distant past, I had been a bit too quick with how I checked pull requests: I’d read the description and then look over each file in order as presented in the changelist. An approach like that is only partially effective. Over time, I’ve learned how to collect my thoughts and separate items by importance. A certain conclusion must be drawn when conducting a code review: “The system can accept these changes as currently presented, and we can handle any complexity that may arise from them. Therefore, I approve.” To get to this state, I put together a little guide to organize reviews and become more efficient with them.
When requested for a code review, I manage it under three principles in this order: context, affordability, and standardization. If I find issues that require new changes, I let the developer know and only continue to the next stage if any additional suggestions would remain applicable. Let’s look at each below.
Context
Context deals with the meaning behind the changes proposed: "Can I understand the purpose from how code translates into written requirements?" Better yet, does the code correctly translate into a description of them? This is the first thing I look for when beginning a review. It is most useful for anyone who has knowledge of the system but needs to understand the reasoning for new changes.
Discovering the context will communicate the “chain of command” within the code: I can see how each changed file connects to one another, from high-levels down into deeper system logic. This helps detect glaring problems or “miscommunications” very early on. If any exist, then the review can be pushed back early to the developer as it likely means the code isn’t doing what it says it should be doing.
How do I try to discover the context? Start reading the files closest to the user or end state of the application, and move downwards to files closer to the internal system logic. A change is more likely to have a more noticeable impact the closer it is to the user or end process. As I trace the logic from those higher order functions down to lower order ones, I can map out the changes more effectively. Context is drawn from seeing how the technical implementation translates between its written communication of changes.
Affordability
It's easy to think that affordability means the monetary cost...but not quite. Affordability deals with total cost of maintaining the changes in the long term, including everything from data collisions, performance, security, scalability, and quality. It limits overall complexity by identifying risks. “How much time and energy do we need to spend on corrections if this code behaves incorrectly?”
Understanding the cost distinguishes those who can anticipate future issues. They can stay one step ahead of the system by having greater knowledge of the full architecture and language paradigms. While I may not have deep knowledge of every possible impact, I can manage what I do know. This principle is necessary for everyone to learn, as it lessens knowledge gaps and opens discussion around possibilities for decay. It takes a village.
Balancing affordability also means finding compromise with the code. It is not about finding things to prevent approval, but instead, is finding solutions that limit unexpected behaviors. Sometimes follow-up work is required when the feature is more important than the assumptions. Sometimes, the work requires additional shaping to be safe. Ask yourself, “Can we afford to manage the identified risks?”
How is affordability uncovered? This part of the process is difficult since it requires delicacy with sufficiently understanding the changes to the system, but can be summed up with few small statements: Understand the context first. Study the system. Read up on the languages used and their own paradigms. Map out the changes the architecture, low to high level. Learn about consequences of any new implementations and dependencies.
Lastly, check for tests as they protect against undesired costs and report on unwanted behaviors. Tests document your system and ensure that previous behaviors remain stable. Advocate for quality, so even when you don't know the entire system, ensure an agreeable level of testing occurred against the new changes, and call out untested behaviors.
Standardization
These checks are the most noticeable within reviews, since standardization deals with agreements on following established code patterns and paradigms. Most standardization issues can be automatically detected through tools like linters and type checkers. However, suggestions here can get a bit opinionated when tools or prop standards for patterns are not already established for the system.
For instance, I may find that a functional programming approach may work better than using long for/each loops, or that case statements are easier to read over complex if/else blocks, or that certain method names are awkward. Others may have differing opinions.
This is why standardization is the least important part of my pull request review: it can be automated or is initially agreed upon as a "contract" when making a change, but anything else depends on managing opinions. Having written prop standards removes bias and reduces variance in the code: in essence, they're similar to requirements themselves.
After I understand the context and affordability, I then scan the files more thoroughly for standardization quirks. Cleaning up code is a chore, or task, that can be done once the higher priorities are discussed.
I still find this important to do, though, as having code that can look the same throughout the suite makes it easier to manage, read, and document. It creates a visual language that describes the system. Variance and customization should be necessary when standards do not exist for describing a given code style, or other approaches have been found to be less effective.
Conclusion
One last note is that I don’t need to end a review as soon as I find an issue with the context or affordability, unless it is considerably large and may require reorganization. Instead, I will still read over and try to clean up or make suggestions along the way. Some suggestions don’t need to hold up an approval, either, so it is wise to make calls based on what is necessary and what is nice-to-have. I’ve been trying to adapt conventional comments a bit more, but without them sounding too sterile. It’s ok to leave a bit of personality in your comment style.
Pull request reviews come down to looking at these three principles in order: defining the context behind the changes proposed and how they translate into a written description, the affordability of the total cost of maintaining the changes in the long term, and ensuring standardization on following established code patterns and paradigms. Doing this has helped me become more efficient in understanding the system through the changes made to it.
I'm always curious how other people handle code reviews. Please share your approaches as I am happy to answer!