A comprehensive guide to perform effective code reviews for Android developers

Photo by Fatos Bytyqi on Unsplash

Code reviews are a part of most devs’ day-to-day work. Despite being a standard industry practice for many years, there are still lots of disagreements around how code reviews should be conducted. In this post, we’ll look at how our framework helps us get code reviews done efficiently.

The authors of this post are currently working on the logistic apps of Lazada, part of Alibaba family. Our current article does not aim to provide a unified framework for code reviews, but to throw some ideas into the dark, and give a hint about how we think code reviews should be done at Lazada.

Stop nitpicking

The first reviewer points out a dozen of things:

“Can we replace ArrayList with HashMap for constant-time lookup?”

AFAIK, there are only 10 values that should be held in that list, so ArrayList is fine in this case actually.

“Can we kotlin-ify this sort of logic? It would look better”

Hmm, good point, but can we have it in another PR?

“Can you rename these variables, please? They are not consistent with other classes.”

They’re actually not in the scope of this PR. Will open another technical debt.

All issues are cleaned up. Few more technical tasks are added. And surprisingly, more nits are commented on.

We both agree that we don’t like comment nitpicking, even though they’re good for the overall codebase. But at the same time, it is less harmful to the team spirit, and time-consuming as well.

Code review is the last stop before shipping the product to people who need it, so as long as it doesn’t make any possible impacts on deliverables and team overall progress, leave a suggestion, approve. If it needs more attention, verbally communicate with others, make a team agreement. As Jorge Castillo rightly said, “Instead of a perfectionism jerk, be an unblocker”. It doesn’t make any sense to block progress by just a variable name. Or as our colleague, Nick Skelton puts it, “if a PR is full of nitpicking, you are winning”.

Automate all the things

  • Checkstyle: we can define the style we want for our code (and we heavily advise using the standard that Android Studio provides). Code reviews focusing on whether there are more or fewer spaces should not happen.
  • Static analysis: Android developers have tools like Lint or klint that can provide us with static analysis of our code. Here we can tackle petty things (such as unused imports, deprecated methods being called, etc… ) or even more serious ones (potential bugs). The authors particularly recommend Lint and klint, since they are the most standard tools for Android development and cover a decent chunk of what we expect in a static analysis.

Protip: CI/CD should be an intrinsic part of a Code Review

We should not forget that a code review is a manual (or human) step in the entire process of building software. An intrinsic part of it is to have it well integrated within a CI/CD process.

Review code, not people.

Be focused and productive during code review

The more complete a PR is created, the better. It is common to find PRs that include just a title, but we can achieve for excellency by adding a few more things:

  • Comprehensive description: describe what the PR is doing, what you have tried before, how was your approach to reach that code, problems you encountered if it is generating any technical debt.
  • Provide a link to the ticket describing this issue.
  • Provide screenshots of the resulting UX if this PR contains any. Or even better, upload a video showing how the app now interacts (GitHub allows now videos).

Considering this simple template:

So [WIP] tells us this is not the immediate PR. [#FBI-23]navigates us directly to the ticket and the checklist hints at what the author wants to achieve in this PR. (For other business-specifics, they should be already defined in the ticket description, if not, please contact product guys)

Protip #1: Issue or pull request template can be configurated on Github. With only few steps, you can add it to your project and code contributors can see it when they open the PR.

Protip #2: Most of your code repos (GitHub, Bitbucket) allow you to set up regular expressions to jump directly into the ticket side. By setting FBI-XZY as a regular expression, each FBI-XYZ becomes a link that will navigate towards your Jira or similar page. This is quite handy and allows the code review to flow dynamically.

Tools can help

Built-in functions

Protip: Fork is one internal recommended tool used in our company. It has provided great experiences so far. (Non-affiliate)

Photo on Fork

The authors have found out that Fork works especially well when we start a pull request. Here is an example workflow

  • Update code base with a single click which is the combination of stash, rebase, re-apply. Resolve the conflicts if any.
  • Stage changes, one file at a time.
  • Commit, push and create the PR.
  • Review one more time on created PR

At this point, your code is actually reviewed 2–3 times before it gets reviewed by others.

Collaboration

Avoid human mistakes

Summary

This is the joint article with Enrique López-Mañas. He writes his thoughts about Software Engineering and life in general on his Twitter account. And this is me on Github, I build and publish stuff there! You can also find me on Twitter. If you have any questions for us, feel free to drop a message or leave a comment. Happy reading and remember to 👏 if you have liked the post so far.

Software Engineer, occasional speaker, product geek, simple things builder.