Today, we continue the conversation with Lydia, who dives deeper with us to share her Fast approach to code reviews.
How do you approach a code review?
I take a quick pass to ensure I understand the overall problem and the high-level approach. If the pull request is large or contains many non-dependent things, I’ll ask them to split it up first. Then, I go through it file by file, line by line.
I use GitHub’s start review feature and leave myself comments. This allows time for answers to become evident or to gain understanding why the engineer made a particular choice without triggering notifications before I'm done.
How do you determine how rigorous each code review should be?
Decide first if the project is high-stakes or experimental, then consider whether the project is:
- Foundational and something that will become a core dependency for lots of other work
- A one-off that can easily be changed
- Something that builds upon an existing framework
For example, if you’re designing the core database interaction layer, those pull requests require a higher level of scrutiny than adding a new user setting.
Since I’ve worked in the identity and access management and compliance spaces, any work on these teams for core features is always high-stakes – and the level of detail I provide and expect on my own pull requests reflects that.
But I think it’s reasonable for a growth team who needs to move quickly and is largely working in experiments to take a more iterative approach to their designs, and prioritize speed over long term perfection and scalability – as those experiments often get sunsetted pretty quickly.
What do you always look for during a code review?
Readability, performance issues, security implications, edge cases, maintainability, and potential side effects. While I am always on the lookout for bugs, I know from experience that even very senior engineers can miss them, so I expect to see unit and/or integration tests covering the core logic to prove it works as intended instead of relying on human eyes to catch things.
How do you know when to stop reviewing?
I think code style is incredibly important in terms of ensuring that the code base is readable and consistent – which allows anyone to jump into a piece of unknown code and quickly figure out what is going on.
But there’s a tradeoff between velocity and perfection. Take that into account to calibrate your line between nit and blocking issues.
What kind of feedback would you give to an engineer new to Fast?
I spend more time explaining my reasoning for those newly hired. For example:
- Why convention X is preferred over convention Y
- Linking to documentation, like external and internal tech specs or style guides
- Finding the exact place in the code where I think their changes might cause a bug and explaining the conditions that could lead to that
- Talking through the trade-offs between two different approaches
- Calling out when something is my personal preference versus established best practices and norms
- I will also spend extra time thinking about bugs that could be caused from edge cases or side effects – things that require a deep understanding of the system at hand. While this is always important, I think it’s fair to assume that someone new to the company might not have the context necessary to catch those issues, so as the reviewer, I should try to compensate for that.
What’s a particularly good or bad code review you've received?
One of the worst reviewers at a past job would say, “Remove unnecessary white space” and “Change X value” in a very prescriptive way, then block pull requests until I made all changes, forcing me to ask for another review. It made me feel like they didn’t trust me to make the minor changes they requested, and didn’t give me an opportunity to explain my reasoning or think through the issue on my own.
Another engineer at the time posed questions, like “What happens if…” in a way that made the code review feel like a real conversation, and prompted me to critically think through my own code which was incredibly helpful as a learning exercise. They’d always approve the PR if all their comments were minor style issues, which conveyed they trusted my judgment on whether to make those changes before or after merging.
How have you incorporated those past reviews into your own code reviews?
I try my best to emulate the latter engineer by assuming the author has thought about this problem more than I have, might have important context I’m not getting, and that they are responsible, intelligent co-workers who can be trusted to carefully evaluate my feedback and implement it decisively before merging.
Want to come work at Fast? We’re hiring!
Additional photo credit: Marcus Spiske