Good pull requests are …
- Small, and
- Haven’t been in the oven for too long (Fast feedback is good), and
- Split by subtask for a feature, or enhancement (Aka smaller is better), and
- Include an idea of benefits, risks, how to verify, discuss other approaches and tradeoffs made getting to this one
Code reviews are a most excellent opportunity to share experience about a program or the way a team thinks about work! The goal is help the author grow if you can, to spot things that could bite you … it is most certainly NOT to nitpick!
- Does it work? 😆
- Is it designed well?
- Does it belong where it’s being added? (Would it simplify life or be more appropriately put somewhere else? eg A library or different app?)
- Is it consistent with the rest of the program it’s being added to?
- Spend more time looking at the interesting bits. Some examples of interesting:
- New libraries / dependencies
- Error handling
- Remote calls
- What if your dependency isn’t available or slow to respond? Have we thought about the failure case?
- Is the change easy to understand and written simply? Avoid being clever. Note: This is not necessarily the same as being short, but short and tactical is often better than the long, and rambling. (Possibly a sign that an objection has been broken down well into sub parts?
- “Show me the tests!” (Units, functionals, end-to-ends, or whatever else might be appropriate. Seeing tests brings me joy 🥰)
- Do we need to update docs? (internal or external)
- Are exceptions being captured and an error report being created for issues we can do something about?
- Yes – bad configuration in a client, or module leading to failure
- No – bad user input (eg Somebody entered a birthdate in a form not in the right format causing our input validation layer to fail)
- Sending an error to a log file means it probably won’t be noticed unless an investigation is underway. Mostly informational. Log events might be used during investigations but they’re not a way to hear about something that’s broken in the first place
- Noise: Defend the channel we receive errors because too much noise leads to apathy and fatigue. Given the way we’ve setup error handling in this code, how many reports do we expect to receive for a given condition? And are they actionable …
My favourite code review links
- One Shopify engineer’s list
- Google Code Review Guidelines
- Here’s something from the point of view of the code author (On bringing reviewers joy …)