Posted 8/19/2018
When working with a team, code reviews are critical to maintaining a consistent, high-quality codebase. Hopefully, your project has thorough test coverage and your team has a shared understanding of best practices. You may even use tools like danger or SwiftLint to enforce components of the code review process. But when it comes to architecting and implementing solutions to complex problems, there's no substitute for having another experienced engineer think critically about your approach.
Code reviews, when done well, also build essential redundancy on the team. If at least two people must understand and reach consensus on all code merged into a codebase, the team will be better prepared to assess new requirements and deal with bugs.
So why a 4-step approach? I've found that doing a thorough code review requires multiple passes. If the code you're reviewing is solving any non-trivial problem, looking at the diff on GitHub top to bottom isn't going to be enough to really understand the solution. Having a goal for each pass through the code helps you stay focused and provide meaningful feedback.
Every team should have a style guide, a consistent process, and a shared understanding of best practices. A great starting point on a style guide for Swift is Apple's official API Design Guidelines. There are also several Swift style guides on GitHub you can use as inspiration for your own. The goal is to have continuously evolving documentation of style rules that help your team maintain an internally consistent codebase. As your team finds new situations not covered by the style guide, you can reach consensus and add new rules.
Process is another area that should be well-documented and entirely objective. Examples include git branching approach and naming conventions, links to project management tools, and templates for pull request descriptions.
In the first pass of a code review, your focus should be on ensuring the team's style guide and process has been followed. With tools like danger and SwiftLint, much of this can be automated. Even with automation though it's still worth doing a style and process pass. You may find things in the code you disagree with or are unsure of that haven't been covered in the team's style and process guidelines. Discussing them as part of the code review process will help the team improve the guidelines and eliminate confusion the next time.
The next step is to pull the branch. Run the changes locally and make sure the pull request accomplished its goal. Having the code locally enables you to use the tools in Xcode to assess it as you would had you written it yourself instead of viewing a set of diffs in a web browser. In this step you can find bugs, performance issues, and design inconsistencies that are much harder to see just from looking at the code changes.
Establishing a pattern of doing manual QA on your team as part of the code review process has several advantages. The most obvious is that fewer issues make it to production. But if your colleagues know you'll be doing QA on their work before it gets merged, they will learn to do more QA themselves. This is different from having an in-house QA team, which can often be used as a crutch. If engineers know a dedicated QA team will be testing builds, they may not be as diligent about finding issues themselves because they trust any issues will be caught anyway further down the line. If they know their initial code won't get merged until these issues are resolved, they may be incentivized to do more of their own QA.
By this point, you've found all of the objective issues with the code. The next step is to make sure you fully understand the problem the author was trying to solve and their approach to solving it. Given the approach the author took, do you have suggestions for improving the implementation?
In this pass, you should try to think about the code entirely from the author's point of view. The reason is that even if you think your approach to the problem is better, the pull request you're reviewing may be ready to merge anyway. You may decide as a team that it isn't worth rearchitecting the solution now, even if you all agree there is a better way. Of course in an ideal world, for complex problems the team would have discussed potential approaches together before the implementation got this far.
Now is the time to think through your own approach to the problem. Ask yourself what, if anything, you would have done differently. What are the objective improvements you feel could be made? Could the problem be solved in a more generic way, yielding more reusable code? In Step 2 you determined that the performance was acceptable, but could it be better? Is a similar problem solved elsewhere in the codebase? If so, did the author use similar patterns? If not, should the approaches be reconciled?
Again, adding your own insights only after understanding the author's approach from their perspective enables your team to make smarter trade-offs between code quality and speed. The marginal improvement may not be worth the time for now, and the team may decide to refactor later.
Am I suggesting you go through this process in depth on every single code review? Of course not. Pull requests range from trivial fixes (e.g. move a label 1 pixel to the right) to major refactors and new features. The idea is to break down code reviews into focused passes so we can improve the quality of the feedback we provide. You can also use this approach on your own code. One of the best ways to improve your code quality is to do a thorough review of your own pull requests before sending them to a member of your team.