Android Code Review Checklist

Does the code follow the project’s architectural patterns?

  • Eg. if you’re using MVP, business logic should be in a presenter class rather than an Activity or Fragment.

Does the code follow the project’s coding style guide?

  • This could be a company’s internal style guide, Kotlin Coding Conventions, Google’s Java Guide, etc.
  • Style guide-related issues shouldn’t come up too often during code reviews; if they do, you may want to update your lint configuration or Android Studio code style settings (under PreferencesEditorCode Style).

Is there any code that becomes obsolete after the code changes and therefore can be deleted?

  • Eg. code behind a feature flag that has been launched.
  • This is especially important for mobile since APK sizes should be kept small. If a large chunk of code across multiple files can be deleted, I recommend doing it in a separate pull request so that the deletion doesn’t distract reviewers from the feature implementation.

Are the error messages being displayed user-friendly?

  • If there’s a designer involved, work with them to design error states.
  • In some cases, it may be better to fail silently as long as the error is still being logged. Eg. if an image load fails, displaying an old cached version of the image rather than an error may be a better user experience.
  • If it’s an error the user may be able to fix, are you letting them know? Eg. if a text input only support numbers <= 100 and a user types in 102, inform them that their input needs to be <= 100.

Are remote non-fatal errors (sometimes called “soft crashes”) being logged?

  • If you catch an exception and handle it, your crash reporting solution will no longer log it as a fatal exception, but it would still be useful to know how often it’s happening. Using Firebase Crashlytics as an example, you’d want to do something like this:
...
catch (e: Exception) {
showErrorToast()
FirebaseCrashlytics.getInstance().recordException(e)
}

Should any code be separated into multiple lines for easier stack trace analysis?

  • Android Studio’s Analyze Stack Trace tool lets you analyze any stack trace you paste in and point you to the line that caused the crash. If you have too much going on in one line, it may be hard to pin down which part of the line caused the crash.
  • For example, the following line may throw an exception if some of the strings are null, or some of them aren’t valid representations of numbers, or none of them are greater than 10.

numberStrings.requireNoNulls().flatMap { it.toInt() }.first { it > 10 }

The following refactor makes debugging easier:

numberStrings
.requireNoNulls()
.flatMap { it.toInt() }
.first { it > 10 }

What happens when there’s no network connection?

What happens when an API call fails?

What happens when there’s a slow network connection?

  • You can test this on an emulator by going to its extended controls and choosing different network types and signal strengths.

What happens when the device is rotated?

What happens if some permissions that the feature requires are denied?

What happens if a user inputs something unexpected?

  • Eg. an invalid email.

What happens when there are edge cases in the data?

  • Eg. if the feature includes displaying a list, how does it look when the list is empty or very long?

Most of these are only applicable if there are UI changes.

Disclaimer: I don’t do a thorough device size and display size check for every pull request, since they can get tedious and rarely reveal major issues. I recommend doing them for changes to critical flows (eg. login or payment). For less critical flows, it’s worthwhile to go through them once in a while on different device sizes and accessibility settings to make sure they still work as expected.

If you have designs provided by a designer, does the UI match them?

  • Make sure you check the loading and error states too!

Are the correct view classes being used?

  • Eg. if there’s a TextView, does your project have a custom extension of TextView that should be used instead? Or if the project follows Material Design, are you using a MaterialTextView?

Do the views and styles follow the company’s design guidelines?

Are you using fonts and colors from the company’s branding?

How does the feature look on a small device?

How does the feature look on a tablet, if supported?

How does it look in dark mode, if supported?

  • You can turn on the dark mode by going to SettingsAccessibilityDark theme.

How does the feature look with non-default font and display sizes?

  • To try it out, you can change these sizes on your device by going to SettingsAccessibilityDisplay.

Does the XML code support usability and accessibility?

  • Eg. are string resources in the correct folder for internationalization?
  • Eg. do EditTexts have the correct inputType?

Is the code testable?

  • Ideally, the bulk of the business logic is in non-Android classes and therefore testable with simple unit tests.

Can classes that were added or modified be easily mocked?

If the project uses UI testing, should any new UI tests be added and existing ones updated?

Are there any tests that become irrelevant or inaccurate after the code changes and should be deleted or updated?

Are threads used correctly?

  • Eg. long-running tasks like API calls and database queries should be deferred to background threads to avoid blocking and freezing the UI thread.
  • Eg. threads are being cleaned up and not causing potential memory leaks.

Is there object allocation or other code in frequently-called functions that can be moved somewhere else?

  • Eg. onDraw() can be called up to 60 times a second in the worst case, and initializing too many objects in it can cause dropped frames and a janky UI.
  • If you have something like:
CustomView : View {
...
override fun onDraw(c: Canvas) {
c.drawPaint(Paint(Color.RED))
}
...
}

consider refactoring to this:

CustomView : View {
val redPaint = Paint(Color.RED))
...
override fun onDraw(c: Canvas) {
c.drawPaint(redPaint)
}
...
}
  • Similarly, an adapter’s onBindViewHolder() may be called frequently, so any code that doesn’t need to happen on every bind should be moved to onCreateViewHolder() instead.

Are there any API responses that can be cached, in order to avoid high data usage and battery drain?

Are there any API requests that can be done less frequently?

This section is mostly relevant if the code changes involve adding new dependencies on external libraries or bumping an existing dependency by a major version.

How necessary is it? If the project only needs a small piece of the library’s functionality, would it be better to reimplement it yourself instead?

How much does it add to the APK size and is the size increase worth it?

How widely is the library used in the Android community and is it actively maintained? Who’s maintaining it?

  • Ideally, it’ll be maintained by Google, a company with a good record of maintaining open-source projects (eg. Square), or an active open-source community rather than a single person.

Does Google recommend using the library or an alternative?

  • If you’re not using the Google-recommended solution, make sure you can articulate good reasons for doing so. Google does change its mind a lot so recommendations should be taken with a grain of salt.

If there’s a version bump, does the new version introduce any breaking changes?

If the project uses Proguard or some other code obfuscator, does an obfuscated build of the app still work as expected or do the obfuscation rules need to be updated?

  • A lot of apps have Proguard enabled for release builds only, and testing on a debug build isn’t enough to reveal Proguard issues.

Are there any places where lambda parameters should be declared explicitly?

  • Kotlin’s scope and collections functions are great for expressiveness and conciseness, but it’s easy to go overboard with conciseness and end up with confusing code. If there are many its in the same block of code, it can be hard to understand what a particular one is referring to and what the block is actually doing. Naming parameters is useful when there are nested scope functions (let, run, with, apply, and also).
var1?.let {
var2?.let {
println("${[email protected]} $it")
}
}

would be more legible with a refactor to the following:

var1?.let { var1 ->
var2?.let { var2 ->
println("$var1 $var2")
}
}
  • They’re also useful for chained calls on a collection to make it clearer what the parameter is exactly at different stages of the call chain.
userIds
.map { idToUserMap[it].posts }
.filter { it.likes > 20 }

would be more legible with a refactor to the following:

userIds
.map { idToUserMap[it].posts }
.filter { post -> post.likes > 20 }

Are resource annotations used where appropriate?

  • Eg. when there are a multiple resource Ints, annotations help clarify which type of resource each one is:
data class Section(
@StringRes val name: Int,
@DrawableRes val image: Int
val count: Int
)

Do the code changes follow the project’s naming conventions?

  • Eg. if the existing view model classes are all named SomethingViewModel, any new view models added should follow this pattern.

Are there any places where using named function arguments would be helpful?

Are there any places where explicit types would be helpful even if the compiler doesn’t require them?

  • Eg. if you have something like val title = getTitle(), even though the compiler can infer title’s type, it may be unclear to another developer whether it’s a String, String?, or @StringRes Int. By specifying the type explicitly like val title: String? = getTitle(), you save other developers from having to look at getTitle()’s function signature.

Are there any places where explicit types are used unnecessarily?

Are there comments that should follow documentation engine formats like Javadoc or Dokka?

  • Eg. if you see a plain comment like:
// Draws as much of the specified image as is currently available
// with its northwest corner at the specified coordinate (x, y). 
// X represents the x-coordinate of the top left corner in pixels 
// and y represents the y-coordinate of the y-coordinate of the top 
// left corner in pixels
fun drawImage(img: Img, x: Int, y: Int) {
...
}

you may want to update it to the following:

/**
* Draws as much of the specified image as is currently available
* with its northwest corner at the specified coordinate (x, y).
*
* @param img the image to be drawn
* @param x the x-coordinate of the top left corner in pixels
* @param y the y-coordinate of the top left corner in pixels
*/
fun drawImage(img: Img, x: Int, y: Int) {
...
}

Are there any comments that are no longer accurate after the code changes and should be updated?

Here’s the link to the PDF version of the checklist.

As a meta-comment, if time permits, I like to let a code review sit overnight as a draft and review it myself the next morning with fresh eyes before officially requesting a review. You’ll get a better sense of what it would be like to see your changes as someone reading the code for the first time. You’ll also likely catch minor issues that you didn’t see the day before.

Here’s Michaela Greiler’s Code Review Checklist again, which I highly recommend reading in addition to this article.

And here are some additional code review resources I like:

Happy code reviewing!

Thanks to Russell, Daphne, and Kelvin for their valuable editing and feedback ❤️