Code Reviews
What you contribute to a code review will tend to depend on your seniority and familiarity with the codebase.
Why do code reviews?
They really help to improve the overall standard of code in your repository, making it much more maintainable in the future.
As a senior developer, they can be a great educational tool, a time to give constructive criticism of someone’s code and help them start to think about the same things themselves.
Another benefit is they can help to distribute knowledge throughout your team. There is value here in reviews from more junior, or less experienced colleagues
Draft vs Final Code Reviews
A very important distinction to communicate to a reviewer is; which type of review are you seeking? Draft or Final? (Corresponding to your belief of the state of the code)
The two types give very different expectations of implications:
- What level to hold it to?
- Is it complete? / Does it contain everything in your definition of done?
- Is it as polished as it needs to be?
One might, for instance, not worry if there are missing tests, or code that clearly doesn’t work in a draft code review.
In a draft code review, you’re often looking for more high level directional guidance. E.g. you might demonstrate a sketched out implementation of the interfaces of the code, without completing the inner workings
Misalignment of the expectations can lead to wasted time, and potentially risking damage to the confidence the reviewer has in the developer, at least in the short term.
The majority of the following points are targeted at Final Code Reviews unless otherwise specified
What should NOT be part of the code review?
Design discussions
Final Code reviews are too late for design discussions on large/complex tasks. No one wants to redo/scrap a large amount of work, so don’t spend ages working on something in isolation and wait for the code review to see if others are happy. This doesn’t mean that design should be by committee from the very start, you should think about a design (possibly many) as part of your grooming, but then seek feedback on it before too much time investment. Set yourself up for constructive feedback e.g. having a mocked up the design to discuss.
Testing
Your code should work! It should:
- Do what was expected from the task you set out to achieve
- Pass all your tests (unit tests, integration tests etc)
Do not rely on the review as a form of testing that the code does what it should.
An aspect I have had to deal with as I matured as a developer was the balance of ensuring the code was correct, with the developer owning the work they complete. With too many guard rails, they can feel as though they don’t own the impact of the change, but the reviewer does as the gatekeeper. If you always catch every issue with work someone does, they can end up relying on that, sometimes people have to make mistakes and learn from them. Consider your context here for an appropriate amount of risk to take.
Aspects covered by automation
You can save yourself significant effort and personal relationship strain by leaving monotonous work to the computer, and agreed automation for aspects like code formatting.
Caveat to this; issues with the automation config can arise at a later point from its initial implementation, and the review might server as an example for discussion on evolving the automation to the new context.
So what should be a part of the code review?
Double-checking that the code does what was expected from the task and is well-designed? Yes, design discussions should happen earlier, and this should be caught there, but it doesn’t mean it always does.
Positive reinforcement
Make sure not to just focus on the improvements required, but to highlight the good elements of the code.
Satisfying your definition of done
Teams often have expectations of work done that go beyond just the necessary functional change. You can use this as an opportunity to check your non-functional expectations are met.
This might include:
- Tests (coverage)
- Documentation
- Changelog updates
Ideally, where automation is possible, this should be delegated to it, or checklists can be included as a guide to convey expectation, and improve self serve experiences
Maintainability
Will adopting this change be suitably maintainable by your team considering your context?
Does the code contain custom implementations of functionality available in an open source package? Consider if your team, or the community are better suited to maintaining that part of the code.
See Choose Boring Technology and Innovation Tokens for some interesting thoughts
Taking on tech debt is a natural part of the prioritisation process, but give it some thought. You might choose not to address an issue in the current code change, as the change provides net value as is, instead deferring an issue/improvement, and referencing the future improvement in a comment linking to the documented expected solution.
Code style
Although as much as possible this should be automated with a CI pipeline. Sometimes though that’s not possible (e.g. with templated files which don’t allow for programmatic parsing/linting).
At a minimum, code standards should be written down, so they can be agreed upon by the whole team.
Readability
Remember code is going to be read many more times than it’s written, so clarity should be critical!
Some common questions to ask yourself:
- Do you understand what the code is doing?
- If not are comments needed? Or maybe something could be more clearly named?
- Is there a clear syntax they could have used (e.g. list comprehensions vs for loop in Python)?
- Does something need refactoring? Is this block of code too complex and needs to be broken apart? There are code metrics for this which can help you spot this
Check for “code smells”
Point out any Code Smells
Domain knowledge
Do I have some domain knowledge which could help improve this code? Or has the developer missed a case you’re aware of?
Business logic
Is the business logic clear, with an appropriate (ideally minimal) level of context?
Is the logic tested?
Consistency with other parts of the codebase
Has a similar problem already been solved elsewhere and can that be reused/combined?
Is the code Pythonic (or equivalent)
Does the code follow generally accepted ways of doing things (in your language of choice) by the wider community, and the style of the language.
Linked CI/CD content
How does the linked CI/CD pipeline look? Sometimes additional information related to the change is included there which helps your review
For instance:
- Does a terraform plan match your expectations from the code change?
- A large refactoring might have significant code diffs, but if the terraform plan shows no changed resources, you have increased confidence in the code change
- Are there potentially unintended side effects?
Performance
Are there some obvious things that might get your spidey senses tingling that a bit of code might not be too fast, or the most memory efficient way to do something. These can be worth pointing out, however performance issues can be complex, so your intuition isn’t always right. The particular bit of code might not necessarily be impactful to the bigger picture. Consider advising measuring its effect on relevant performance to be sure, and suggest options to consider.
How much context should you need for a code review?
In my experience, not a significant amount about beyond the general knowledge level within your team/squad. If the code review leaves you with lots of questions about why something is done in the way it is, take that as a sign that something can be improved?
Maybe: - the code could be structured better to aid understanding - names of things currently give the wrong idea - More comments are needed to give context
I like to take a first pass at a code review alone to catch these things, and if things aren’t clear beyond some minor details, consider a synchronous discussion with the author to enable more constructive feedback.
If however, you gather the additional context first, with the aim of a smoother first code review, you can let issues go unaddressed that could trip up the next reader, who is missing the context you temporarily have front of mind.