Code Review guidelines
What is a Pull Request?
A Pull Request or short PR is a mechanism to review and comment on features, bug fixes, or improvements added by others.
Although the mobile team at VMLY&R Poland is divided into two platforms developing apps using mostly native Kotlin or Swift, we also try to improve our skills by discussing the second platform. It allows developers to validate if the business logic has been implemented correctly by looking at unit tests or finding minor language-related issues.
There are many sources we refer to but pasting a few important ones:
- Clean Architecture Robert C. Martin
- iOS Unit Testing by Example: XCTest Tips and Techniques Using Swift Jon Reid
- Clean Code Robert C. Martin
How many people should approve
We don’t rush and value quality over speed. If you want to develop your skills more and more, getting positive or mostly not feedback about your code is necessary. It should be your initiative to discuss parts you’re unsure about with more experienced fellows.
But if you ask for a specific number for our team of 6 engineers? At least 2 of them.
How long do I need to wait
Reviewing teammates’ code should be a vital part of your work. It’s constructive if you do it immediately, but save some space in your calendar and do it at least once per day. If you suspect your colleagues are not engaged in the code review process, please gently remind them.
It doesn’t look good if you see a pull request hanging unrevised for more than two working days. Inform a project manager about that, and both improve the team spirit.
No matter how brilliant your mind or strategy, if you’re playing a solo game, you’ll always lose out to a team. — Reid Hoffman
Working in parallel
You should have max. 2 Pull Requests (PRs) assigned to yourself. Depending on task priority, you should resolve all issues and comments as quickly as possible before picking another task so as not to block the rest of the team.
The team also needs to specify the maximum number of PRs for the entire team. Following PRs will be automatically rejected. Our squad of three devs working on a single platform has a maximum of 3 PRs.
Comments
We keep three types of comments — green, yellow and red ones. Seniors will give both praises as well as constructive criticism! Juniors are more passionate about implementing new features. Great things can happen when you can hear other voices!
✅ Green one
Give praises! Seeing an approbation can be vastly motivating, and keeping a good attitude works for us.
Examples:
✅ Good usage of a strategy pattern
✅ Glad I read it. I wasn’t aware we could solve some problems in such an easy way.
✅ I’m more interested in this solution. @Author, can you link a reference to read more about it?
⚠️ Yellow one
They should be considered as a warning. You can point out what can be improved, but it’s not a blocker.
Examples:
⚠️ Too many enters.
⚠️ This code can be simplified using …
⚠️ Consider renaming this variable to be more meaningful. I couldn’t understand it from a context.
⛔️ Red one
They prevent PR from being merged. PR author needs to resolve all of them or discuss it with a comment author to understand the scenario better and come up with a solution. It’s a PR author’s responsibility to bring the discussions to an end.
Examples:
⛔️ Provide unit tests for these classes: …
⛔️ Replace force unwrapping with guard let statement
⛔️ Divide this class into smaller ones.
Few more general rules
- Add screenshots or (even better) GIFs to UI-related PRs.
- PR does not have to be related to the task. For example, it can simply add just a new HTTP Client implementation.
- Therefore PR must be small and related to more minor issues than big ones.
- There are no size limits for PRs, but be reasonable about that.
- Add them in the description section if there’s a JIRA ticket(s) related to the PR.
- Don’t write what description, but rather why you did it. (Code reflects what you did 🤓) — Instead of
AlamofireHTTPClient class added
use
I’ve added HTTP Client implementation to Alamofire since the existing one has issues with background tasks and can cause memory leaks. - If you work on a vast task, consider creating an epic branch and making smaller PRs to that epic branch. It’s easier to check, and it’s easier to merge with the development branch in the future.
