We at Icinga / NETWAYS (yes, that’s the order) held an internal event recently. It’s name was Knowledge Days and I got to to talk about how I review code. Now, I will share my knowledge with you!
Though, this is specifically how I personally perform reviews. This is by no means the definitive way of doing it! Find your own, I can only share my experience. So without further ado…
As Author – Requesting Review
Every review starts with you requesting one. It depends on the code to review however, who you should ask. Someone might be more experienced than others in a specific area, so ask those people first. Platforms like GitHub suggest reviewers already based on who has touched the same area previously. Sometimes you may want multiple people to review your code. In such a case, it is probably a good idea to tell them in advance what they should prioritize on. This gets more important as the size of changes to review increases. Making it easier to navigate them, speeds up the process significantly.
What will also speed it up, is how the code initially looks like. If you regularly review code yourself, you should know how to prepare changes in a way that the reviewer doesn’t turn away immediately at first sight. Things any author should ensure before requesting review are:
- Documentation – Interfaces, complex blocks and a description for the pull/merge request (Reference the issue it is related to)
- Formatting – There are official guidelines on how to format code for most languages. There may also be internal guidelines
- Checks – On GitHub and GitLab might be a continuous integration pipeline set up. It should succeed. If it doesn’t, you should leave a comment explaining why
- No Conflicts – A branch with conflicts cannot be merged. So why should anyone review it? Seriously, fix any conflicts before requesting review! You might miss an incompatibility
- Test – Perform your own tests. It should go without saying, but if a proposed change doesn’t do what it’s supposed to…
As Reviewer – Where to start?
You have to be aware of it first. Make sure to regularly check for review requests. Then, take a first look early on. Avoid long delays, an author might need to get familiar again with the changes if the proposal is very old. Or even worse, someone else needs to take over.
During the first look, check the prerequisites I have mentioned above. Don’t waste your time if something is not right. (The author did not either, after all) Simply leave a comment and wait for a new review request.
Perform some tests. If the change addresses a bug, check if it is fixed. If it is a feature, verify it works as intended by checking the related issue. (Given that this information can be found there)
As Reviewer – First Reading
In case you did not spot any obvious flaws during the first look, it is time to establish an overview of the changes. This is again more important as the size of changes increases.
- Identify Additions – Such represent only dependencies and do not influence existing functionality
- Identify Changes – Those might influence existing functionality and introduce bugs or break interfaces
- Allocate – Applications have various areas. It is sometimes helpful to isolate changes from another to review them independently
Now, take a deep dive. You know the general structure of the changes which should aid in taking a detailed look. Keep the bigger picture in mind. Changes might be intertwined somewhat, which is the number one reason for hard-to-catch bugs, if there is something wrong.
The documentation might be perfect and the code relatively trivial, but there can still be something you do not understand. Do not hesitate to ask the author about it! Debugging is also a great way to understand how code functions. Either by utilizing a real debugger or by simply trying to evaluate it in your mind. (If you can, you are a real pro 😎️)
Things you should pay attention to:
- Readability – Good variable names, helpful comments…
- Structure – File placement in the proper namespace/module/package
- Maintainability – The changes might benefit future ones
- Safety – There might be vulnerabilities
- Efficiency – Loops are not always great…
If the change affects multiple independent areas of an application and is really large, consider giving feedback in batches. This allows an author to adjust the code in parallel, while you take a look at another area.
As Reviewer – Good Feedback
You are communicating. So, keep your manners.
- Constructive – It is not always about criticism. If something deserves a compliment, do not withhold it. If you already know the solution, do not keep it
- Precise – Avoid misconceptions. If you have a particular expectation, tell it the author. Otherwise you will surely be disappointed
- Objective – Guidelines cannot cover everything. And should not. If you cannot argue objectively, let it be
As Author – Accept Criticism
You are also communicating. If you make an adjustment based on a reviewer’s comment, react accordingly. In case the reviewer suggested a specific solution, explain what you have done differently still. If you did exactly what was suggested or simply took action, tell this the reviewer as well. Though, please avoid resolving a comment to do this. On GitHub and GitLab this leads to this comment being collapsed and hence makes a followup review inconvenient. Resolving a review comment should be the comment author’s responsibility.
Feedback Loop
This repeats until the reviewer is satisfied and approves the changes. It might take a while to get there, but this is perfectly normal. But if the author and reviewer often argue with another, it might be time to resolve a very different type of conflict. 🤬️
Best Practices
- Fewer Changes – Smaller pull/merge requests have a higher chance of being properly reviewed and tested. They also increase interoperability with other changes as they are merged earlier
- Linear History – During the review process, an author should avoid force pushing. The reason is that GitHub tries to visualize differences between consecutive reviews, which fails if the commit history changes
- Once a change is approved, the commits can of course be re-arranged (squashed) and in case that is done properly, the review approval is not automatically revoked
- A force push might also be mandatory, as a rebase was necessary, this should just not happen regularly
- Continuous Integration – GitHub and GitLab have both CI pipelines which can be used to automatically test changes regarding syntax, formatting and unit tests. Do yourself, either as author or reviewer, a favor and set this up
