iOS: Code review practices
I am pleased to share a list of concerns we should care about when doing a code review in iOS. Hope it gives you a direct approach and is helpful in software development.
As software engineers, we create pull requests and process code review daily. But sometimes because of workload, meeting or deadline we didn’t review code properly.
Have you ever…
- Felt upset because no one reviewed your pull request?
- Felt frustrated because reviewers require to polish every tiny piece of a change list before granting approval?
- Felt lazy while seeing a big pull request of your colleagues that need you to review?
- Approved a pull request automatically without reading complex changes?
Some useful abbreviations in this article:
- Change List: CL
- Continuous Integration: CI
- Pull Request: PR
- User Interface: UI
According to Google’s Engineering Practices, we have a list that code review should look at:
- Design: Is the code well-designed and appropriate for your system?
- Functionality: Does the code behave as the author likely intended? Is the way the code behaves good for its users?
- Complexity: Could the code be made simpler? Would another developer be able to easily understand and use this code when they come across it in the future?
- Tests: Does the code have correct and well-designed automated tests?
- Naming: Did the developer choose clear names for variables, classes, methods, etc.?
- Comments: Are the comments clear and useful?
- Style: Does the code follow our style guide?
- Documentation: Did the developer also update relevant documentation?
Code review time spending vs appreciate level 😊
By collecting from the internet and my own experience, I would like to share some practices of the code review process in developing software products. I try to list code review steps based on how much time we have for reviewing code.
If you have
5 minutes 👍
- Check CI build report if it has any errors or failed test cases?
- Check SonarCloud (if available) scan report if it has any code smells, duplications, enough unit test coverage?
- Unused code, comment code should be removed.
- Does it have any unlocalized strings?
- Make sure the CL doesn’t have any magic numbers, specifically for UI implementation.
- Font, color should be declared at a generic place for reuse.
- Coding convention style: spacing rules, break new lines rules, comment, naming convention (uppercase, camel case, snake case)
- Avoid force cast and unwrap optional value. Make sure no implicitly unwrapped optional exclamation mark appears !
10 minutes 😁
- Does the CL miss any [weak self] or [unowned self], which can cause a retain cycle?
- Does the CL include any nested ternary condition, multiple level of if-else, switch case, loop?
- Does any if-else implementation missing else case? Reviewers should ask questions to make sure that is the author’s intention.
- Avoid public IBOutlet, IBAction just in case of author intention.
- Avoid updating layout constraints when the UI component is hidden/shown. Please use stack view instead.
- Ask the author for a few screenshots if there are huge UI changes.
15 minutes 😍
- Does the CL follow a single responsibility principle? Any class or function should have only one duty.
- Is the CL able to extract into a small and pure function?
- Is the CL “too complex” or “over-engineering”?
“Too complex” usually means “can’t be understood quickly by code readers.” It can also mean “developers are likely to introduce bugs when they try to call or modify this code.”
A particular type of complexity is over-engineering, where developers have made the code more generic than it needs to be, or added functionality that isn’t presently needed by the system. Reviewers should be especially vigilant about over-engineering.
Encourage developers to solve the problem they know needs to be solved now, not the problem that the developer speculates might need to be solved in the future. The future problem should be solved once it arrives and you can see its actual shape and requirements in the physical universe.
— Google’s Engineering Practices —
- Does CL follow project architecture (MVVM, VIPER, RIB, …)?
- Does CL locate in the right place: main codebase, library, or any specific module?
- Does the CL have a common dependency that was already implemented before?
20 minutes 😘 (author may buy you a coffee ☕️)
- Double check if business logic is implemented correctly.
- Does CL have any race conditions such as asynchronous events in a loop?
- RxSwift: Do any errors bind to a relay?
- RxSwift: Does dispose bag deallocated before subscriber trigger?
- RxSwift: Does dispose bag re-initialize when cell prepare for reuse trigger?
- RxSwift: Is the UI implementation subscribed on the Main Scheduler? Try to use Driver instead.
- RxSwift: Does Observable subscribe many times?
Conclusion
According to the time you have (while waiting for Xcode indexing, Carthage update, pod install ⏳), you can help others improve code health by going through the checklist above.
Code review is one of the opportunities to learn skills from each other.
Be kind in comments, don’t make the author upset 😭.
There is no such thing as “perfect” code — there is only better code. Reviewers should not require the author to polish every tiny piece of a CL before granting approval. Rather, the reviewer should balance out the need to make forward progress compared to the importance of the changes they are suggesting.
Instead of seeking perfection, what a reviewer should seek is continuous improvement. A CL that, as a whole, improves the maintainability, readability, and understandability of the system shouldn’t be delayed for days or weeks because it isn’t “perfect.”
— Google’s Engineering Practices —
Read more from here
