Wikimedia Apps/Team/Android/Development philosophy

This document outlines the general philosophy of our development practices in the Android team (responsible for the Wikipedia Android app), and a few high-level guidelines for effective and successful Android development for the present and the future.

Rapid development

Because the size of our user base is much smaller than mobile web, yet still large enough to have an impact for many crucial audiences, we have a unique opportunity to be much more experimental in the types of functionality we provide. Our team can be (and has been!) an incubator for new ideas, to be piloted on our platform, and then either adopted on other platforms (e.g. mobile web), or retired quickly, with feedback gathered and lessons learned.

This means that we need to be able to move unusually quickly in our development cycle. That's why the things I'm going to say might sound heretical to a lot of other engineers. The first thing of this sort is: dogmatic adherence to conventions (e.g. rigid style guidelines, percent-targets for test coverage, goals for lint-free code, etc.) can easily become a hindrance to this philosophy, instead of a benefit.

How strict is too strict?

Let's get real for a minute. We are not building firmware for nuclear centrifuges; we are not building an air traffic control interface, nor are we building a Mars rover. We are building an Android app for reading Wikipedia articles. That's not to say that one is more "important" than the other, or that one isn't as technically challenging than the other, but...

Strict adherence to coding conventions, test coverage, etc. should be proportional to how far upstream you are, and how mission-critical your product is. If you're submitting a patch to the Linux kernel, then it makes sense for every character of your code to be scrutinized as much as possible. If, however, you're building a new feature for a mobile app that displays Wikipedia articles, the rules can surely be relaxed somewhat.

We are not writing code that will need to be maintained by future generations of developers twenty years from now. Everything about our platform is ephemeral. New languages will spring up, new frameworks and libraries will be adopted, new designers will redesign the entire interface, and the entire platform can one day disappear from under our feet, to be supplanted by a new and better platform. And all of that is OK!

I'm by no means advocating for an abandonment of best practices and norms, but rather for a better balance of reasonable guidelines and an awareness that our product is a perpetual work in progress with an acceptable failure rate. This is the key to rapid development with limited resources.

Code review

Reviewing someone else's code should consist of two questions:

Does it work as expected and (if there's a user-interface component) look as designed?
Is the code semi-competent?

The next few sections will explore what is meant by "competent" code. But first, a few words on the code review process itself.

The comments you leave when reviewing code should be as positive and constructive as possible. Engineers have a natural tendency towards defensiveness when something they created gets criticized or otherwise "reviewed" by someone else. Since code reviews are restricted to plain text, it makes them vulnerable to a loss-in-translation effect where any nuance, intonation, or emotion of the reviewer is lost.

Therefore one must be especially mindful of maintaining positivity when leaving comments in code reviews. Put yourself in the position of the reviewee, and consider how you would feel if you received the comments you're about to give. Try to formulate your comments as questions instead of statements. For example, instead of saying "This block of code is utter nonsense and should be rewritten," try saying "Would it be better if we restructure this code like this: ___?"

Conversely, you should never hesitate to request changes on a patch if something doesn't look right, even if the reviewee is an engineer that's more senior than yourself. And you also shouldn't expect a patch of yours to be merged on the first pass, without one or two follow-ups.

And once again, when reviewing code, keep in mind all of the philosophies outlined previously (rapid development and all that), and try to limit your insistence on code style perfection, especially if the code is working properly, or if the argument over style is getting in the way of real development of the next meaningful feature.

Merge first, tweak later

If you're reviewing someone's patch, and the functionality works as expected, and the code looks semi-competent, but you notice some minor style tweaks that can be made, here is what to do:

Merge the patch!
Submit a follow-up patch yourself that contains the style tweaks, and make the author of the original patch review it.

This will speed up development in the following way:

When Alice works on a new feature, she creates a patch that implements it, then submits it for review, and then moves on to something else. She switches contexts, and her mind is no longer in the context of the feature that she just completed.
When Bob reviews Alice's patch, he gets into the context of that feature. He also very likely has the code already checked out locally. Therefore, at that moment, Bob is the best person to make additional changes to it. So, if Bob has some minor style or optimization comments regarding Alice's implementation, he can make the changes himself as he reviews the code, and submit a follow-up patch right then and there.
When Alice comes back and reviews Bob's follow-up patch (when she's ready to switch contexts again), she can learn Bob's reasoning for the follow-up, and draw lessons that she can apply to her future work.

We are all product owners

Our team has a dedicated official product owner, but guess what: we are all product owners. The product belongs to all of us, and we are responsible for it, and accountable to our users. To that extent, we should all be dogfooding our product, i.e. using it in our daily lives. If the product is not for us, then who is it for?

We should be quietly but persistently encouraging our friends and family to use it, asking what features they would like to see, and getting feedback about their experiences with it. If the product is not for them, then who is it for?

If you're finding that you're not using our app in your daily life, ask yourself "Why not?" What potential feature in the app would make you want to use it? What feature would you be particularly excited about building? And... why haven't you built it yet?

Alright, with all of that said, let's finally dive in to the actual practical guidelines for development of our product, from the specific perspective of Android engineering.

Focus on performance

One thing on which we must not compromise is performance (i.e. speed and responsiveness). Let's not forget that one of the great benefits of native apps is better performance than can be achieved in mobile web. If we let our focus on performance slip, we would be defeating one of our raisons d’être.

Free up the UI thread

Do not write code that does too much work on the main thread. The Android OS helpfully alerts you in logcat when this is happening. We have several classes and constructs to perform asynchronous operations easily, so make sure to use them.

Of course, when an asynchronous operation returns to the main thread, make sure to check the state of the current Activity or Fragment (i.e. make sure it's still active or attached to an active Context).

Minimize and optimize database interactions

Our app uses a SQLite database for storing a lot of things including reading lists, browsing history, thumbnail URL caches, etc. Interactions with this database must be as efficient as possible, so that the user interface remains responsive and seamless (see previous point). The way to do this is to be aware of how SQLite works and how to optimize it. To name a couple of specific examples:

Don't perform multiple queries in a row, when a single better-crafted query will suffice.
When it's necessary to perform multiple queries, or to perform queries in a loop, wrap them with beginTransaction() and endTransaction() calls.

Don't overcrowd View hierarchies

Views that are displayed in an Activity are actually rather expensive objects, and should be used sparingly. When there are too many Views on the screen, it can slow down the rendering process significantly. The problem is compounded when we have a complex View hierarchy inside RecyclerView items (i.e. Views that are repeated numerous times, or nested very deeply). Therefore, keep the following guidelines in mind:

When possible, use the newly-introduced ConstraintLayout, which provides a great deal of flexibility for child Views, and can effectively flatten View hierarchies that would otherwise be badly nested.
When using a RecyclerView, make sure it's used properly: specifically, make sure that its height is set to match_parent, and never wrap_content, so that the RecyclerView doesn't accidentally render all of its children, including ones that are off-screen and don't need to be rendered in the current window.
When creating a custom View, make sure that the root node in your custom XML layout is a merge node (with tools:* attributes to aid laying out the view in the Android Studio designer), instead of an unnecessary root FrameLayout or something similar.

That being said, the importance of minimizing/optimizing View hierarchies might not be as important as all that.

Don't use too much memory

Our platform has fairly limited resources, especially when compared to the nearly-infinite resources of desktop platforms. Therefore we need to be mindful of how much VM memory we allocate. Android Studio has a helpful real-time tracker of memory usage when debugging the app, which can be useful for tracking down memory hogs.

Don't marshal/unmarshal huge objects unnecessarily

Transforming a plain Java object to its JSON representation (in a String) is a pretty expensive operation, and should be done only when necessary. Other more specific points:

Don't send huge amounts of data across the WebView Javascript Bridge. This is a fairly expensive operation, and should be restricted to modest amounts of data.
Don't exchange huge amounts of data between Activities. That is, don't marshal huge objects into a Bundle and send it to an Activity inside an Intent. The Android OS actually imposes a hard limit on the maximum size of data that you can send this way, and will crash the app if this size is exceeded.

Keep an eye on APK size and method count

At the time of this writing, our method count is about 45000. While this is still far from the danger zone, we still need to be mindful of eventually reaching the dreaded dex limit of 65535 methods. Exceeding this limit will be a significant hit to our performance, and should be avoided if possible.

Our current APK size (again, at the time of this writing) is around 23 MB, or 10 MB when split into separate ABIs, which is actually relatively small compared to most other major apps. Still, it’s worth keeping an eye on how our APK size is impacted by new features that we add, or new libraries that we depend on.

Don't create memory leaks

Yes, if you try hard enough, you can create memory leaks even in a garbage-collected language like Java. This can be done by storing references to an Activity or Context in a static variable, among other unfortunate examples.

Write clean code

The code that you write should be clean and understandable. With a little practice, this can become second nature. Clean code leads to faster code reviews, since the reviewer instantly understands what you're trying to do. It also leads to better maintainability, since whoever revisits your code at a later time will understand it faster, and will be able to make updates more readily. When writing code, ask yourself, "If an outsider looks at this code without seeing any of our other code, will they understand what's happening?"

Naming

Be generously descriptive when naming your variables, classes, and functions. There is no performance cost in having verbose names. Providing descriptive names will be a benefit to everyone including yourself, the next time you have to look at your own code.

Give descriptive names to classes, fields, function parameters, even local variables. You'll thank yourself later. The only permissible non-descriptive variable is i, when used as the index of an innermost loop.
Include units of measurement in the name of your variables and constants. Suppose you have a constant called REFRESH_DELAY = 5000. What are the units of that constant? It might be obvious to you that the units are milliseconds, but it might not be obvious to everyone else, so a better name for the constant would be REFRESH_DELAY_MILLIS = 5000.
Give descriptive names to resources, such as strings and View identifiers. If you have a Button in your Activity, don't call it +id/button, but instead give it a name that actually reflects what it does, for example +id/create_account_submit_button.
A function should be named after the thing that it does. Avoid function names like doIt() or getStuff() or cheeseburger(), unless you're entering a code obfuscation contest.
Member variables no longer need to start with the letter "m", or use any other kind of Hungarian notation, because it's 2018 and we have modern IDEs that provide different highlighting to reflect the type of variable you're looking at. mOK?

Classes and methods

A class should have as few responsibilities as possible (preferably a single one).
Ideally, all of the dependencies of the class should be stated in the constructor.

Annotations

The Support Library provides numerous helpful annotations that can make our code more comprehensible, as well as instruct the IDE to alert us to possible issues. Get into the habit of including annotations whenever they are applicable, specifically:

If the return value from a method is not a primitive type, it should be annotated as @NonNull or @Nullable, to make it explicitly clear what the method returns. The same applies to annotating parameters that are passed to methods, to make it explicitly clear what the method expects to receive. Generally we should lean towards enforcing @NonNull wherever possible.
If a parameter is an integer type, but actually refers to a resource identifier, you should use the appropriate @Res annotation, such as @IdRes, @ColorRes and @ColorInt, @AttrRes, and so on.
If you're building a class in which you expect descendant classes to call the super() method for overridden methods, then use the @CallSuper annotation.

Modern language features

Make use of the latest syntactic sugar provided by the language, such as lambdas. However, prior to using such features, do a little research on how they actually unfold under the hood, and what possible performance cost they might incur.

ButterKnife

When creating a new Activity or custom View, use ButterKnife for binding the Java code to the layout resources (using the @BindView annotation). Ideally we should never have to call findViewById() ourselves. Whenever possible, also use the convenient @OnClick and other similar annotations.

Colors and styles

Avoid using raw color values in XML layouts and in code. Instead use @color resources, or better yet, ?attribute resources that point to the corresponding color in the current theme.

Reactive code

We use RxJava for handling asynchronous operations and transforming data flows in a clean way. Here are a couple of pitfalls to keep in mind when working with RxJava:

If you create an Observable by using the fromCallable() method, the result of the Observable is expected to be non-null. If the callable returns null, it will not be passed to the onComplete handler, and will instead cause an error to be thrown in the observable chain. Therefore, if your callable might return null, then make sure to have an onError handler when subscribing.
If you need to perform an action at the end of the subscription chain (regardless of whether there was an error or success), then use the doFinally() method, and do not rely on onComplete(), since onComplete() will not be called if there was an error.
It's good practice to keep the results of subscribe() calls inside a CompositeDisposable object, and then clear() it when the activity is destroyed. However, keep the following in mind: after an observable is disposed, it can still throw errors (e.g. if a Callable returns null), which will automatically go to the "global" error handler specified in RxJavaPlugins. By default the global error handler simply rethrows the exception, causing a crash. For the moment, we are simply overriding the global error handler with an empty handler that will just suppress the error

Unit tests

Ever a source of heated debate among developers, here is my take on unit testing: There are tests that are extremely useful and helpful, and there are also tests that are unnecessary, redundant, and generally valueless. Distinguishing between the two is, as everything else, a fine balance.

Here are some examples of good unit tests:

Our Translation tests (i.e. the tests that verify the formatting of all of our localized strings) are vitally useful, and routinely catch errors that would otherwise lead to incorrect presentation of text in various languages.
Tests having to do with handling and transforming URIs, Dates, PageTitles, and WikiSites. These objects are of fundamental importance to nearly all features in the app, and should therefore be tested exhaustively, especially when dealing with edge cases such as language variants, URL fragments, non-main namespaces, etc.

And here are some examples of meh unit tests:

Screenshot tests. The technology for staging screenshot tests is not yet mature, and requires enormous effort to set up and maintain. Furthermore, the human eye is a perfectly good screenshot test, and since all of us should be using our product in our daily lives (do we not?), this form of screenshot test should ideally be running in real time, for free.
Most of our network client and callback tests. Our network tests consist of feeding a canned response into a network client, and observing whether the correct callbacks are called. These tests are useful only if the format of the network response never changes, which in practice is not guaranteed. And once again, the unit test of the human eye can often pick up regressions when a network client fails.

So once again, the best unit tests are ones that test objects that are upstream, and objects that are used in many different contexts. Whereas bad unit tests are ones that test functionality that is high-level enough to be caught by routine usage of the product. Even worse tests are ones that require regular maintenance, for no other reason than to keep them passing, i.e. tests that get in the way of development.

Patches

The size of patches (or pull requests, if you prefer) should be as small as possible, and the contents of the patch should be as concise as possible.

If you're building a patch that implements a new feature, and then you think to yourself, "Why don't I also fix this unrelated crash as part of this patch," break it up into two separate patches.
If you're building a patch that fixes a crash, and then you think, "Why don't I also add some useful annotations to these unrelated classes," break it up into two patches.

Internationalization

There are many things that we in the English-speaking world take for granted when building software in general. But especially in Android development, we must keep the following points in mind:

Be mindful of Right-to-Left locales (e.g. Hebrew and Arabic) when building layouts and components. After building a new component, switch your device to an RTL locale and see how your component looks. Chances are that you'll need to make a few tweaks to make the component properly "mirror" the LTR behavior with which you're comfortable.
Some languages are intrinsically more "verbose" than English (I'm looking at you, German), and use significantly more letters to represent the same idea. When designing and building layouts, be mindful of the physical dimensions and spacing of your components, and check whether a significantly longer text label would make your component overflow into an ugly mess, or whether the string becomes cut off entirely.

Date formats

Formatting dates in different languages (e.g. "Sunday July 1, 2018" or "5 days ago") is a maddeningly incomprehensible affair, and requires painstaking logic to do correctly. There are entire libraries dedicated to correct internationalization of dates, and fortunately Android provides them for us in the form of the RelativeDateTimeFormatter class, as well as the getBestDateTimePattern() method, among others.
Do not use string resources with parameters for formatting dates.

Plurals

When possible (and applicable), use plurals resources instead of plain string resources. Do not use separate string resources for singular and plural versions of the same text.

Display sizes, resolutions, and orientations

The Android ecosystem is, as we all know, incredibly fragmented. There are many thousands of different models of devices, each having a different display resolution, pixel density, aspect ratio, etc. We have to be mindful of all of these factors when designing and building our interfaces.

Landscape orientation: don't forget to occasionally turn your device sideways and see how your layouts and components look in landscape mode. Make sure that it doesn't look like an absolute disaster, and that all of the components in your activity are accessible (by scrolling, if necessary).
Never use raw pixel units in your layouts or in your code. Instead, use device-independent pixel units (dp), and scale-independent pixel units (sp) for text sizes.
Since recent versions of Android now support split-screen mode (with adjustable split!), we must make sure that our interfaces remain usable when inside a split window.