The title might sound weird at beginning since many modern libraries or tools are designed to make developers write less code via ways like code generation or reflection. Those libraries are great and convenient to use because we write less code. However in my experience sometimes developers become careless and misuse those libraries by calling them everywhere they could. Eventually the codebase becomes difficult to read and maintain. Here are some examples I have seen many times and would like to share with you.

Automatic lifecycle management

I would like to start the content by defining “convenient” and “explicit” so we can all get on the same page. But instead of me explaining everything here, let’s look at a post by Dan Lew: Why Not RxLifecycle? together.

Lifecycle management is tough thing to do is Android world especially when you have to deal with fragments as well. Therefore Dan, as a RxJava enthusiast, created a library called RxLifecycle to help developers to manage subscription automatically. The idea is great, and the library became popular quickly (5149 stars). However Dan later wrote the blog post to discourage the usage of his own library.

Why? You can check out the details in his post. Here I am going to quote some points directly for brevity:

Automatic lifecycle detection leads to confusing and sometimes non-deterministic code.
Often times you end up manually handling the Subscription anyways.

Don’t get caught up by those Rx specific terms. You should be able to grab the idea easily: there’s no one rule for everything, and sometimes you just need to be explicit even if it can be slightly more verbose.

Field Injection

Ever since dependency injection became “the right thing” to do for Android development, I have seen many developers abusing the tools. Libraries like Dagger allow you to do injection in different ways. You can have constructor, field, or method injections. In addition you can directly grab instances directly from object graph. It’s too convenient and flexible so that I keep seeing developers use it wherever they could like:

 1
 2
 3
 4
 5
 6
 7
 8
 9
10
11
12
13
14
15
public class MyAdapter {

    @Inject Dog dog;

    MyAdapter() {
        MyApplication.graph.inject(this);
    }

	void onClicked() {
        runAsync(() -> {
            Cat cat = MyApplication.graph.get(Cat.class);
            // pet the cat
        });
    }
}

The problems in the code above are:

It has multiple injection points, which can be confusing and cause nondeterministic behavior especially when you have different graphs or scopes.
It makes tests difficult to write and depend on the DI library.

My suggestion here is to use constructor injection whenever it’s possible. It forces you to pass in every collaborator so you don’t need to think about where those instances comes from and it does not require special setup for unit tests since you decide what instance to use when creating test objects.

There are many other benefits of using constructor inject. You should checkout the post Keeping the Daggers Sharp by Py, a Square engineer.

Sometimes you just cannot use constructor injection because you don’t create the instances. For example in Android activities and fragments are created by the framework. In those cases I think using field injection is definitely okay, but you should try to limit where you can call inject() method. For example always call it on activity’s onCreate(). (fragments can be tricky if you need scoped dependencies…)

“Global” Event Bus

Okay this is not something new. Many developers have mixed feelings about event bus. I have heard similar stories. Developers start using event bus because it helps “decoupling” the classes. You don’t need to worry about passing listeners down to three or four level down. All you need is to pass “one” event bus around and you can receive callbacks anywhere.

The problem here is obvious that once one event bus “global”, essentially you “couple” every subscriber and sender together in a hidden way. What’s worse is that event bus does not provide explicit contract between components. Everyone can send out any events at anytime and in any order. Threading is also another painful painful thing for event bus system because you have to make sure subscribers are called on desired threads.

It is convenient to use at a small scale, but once the codebase become larger you would want explicit contracts because it’s more maintainable.

Fragmented Podcast has an episode talking about 061: The state of event bus(es) today. You should listen to it.

Objects, maps, bundles

So the last section is all about types. I think being a Java/Kotlin developer, I really appreciate the strong type system because I can easily navigate to the relevant classes or usages of a class. However I have seen some weird usages that try to “work around” the type system:

magical contract:

1
public Object magic(Map<?, ?> map) {...}

Most of times, the function is easy to write either because the author don’t want to deal with generics or they try to “accommodate as many types as they can” in one method.

magical bucket:

In a POJO for json:

1
2
3
4
5
6
7
8
9
public class JsonResponse {

	public String name;
	
	// ignore ten other properties
	
	// for all others
	public Map<String, Object> catchAll;
}

Most of JSON paring libraries allow you to define a map to capture all the key-value pair, so some developers will take the advantage of it and use the POJO for different responses. They try to reuse the same POJO for different endpoint responses. Once they get the object, they would then use different keys for different endpoint responses.

This can be confusing because you hide all the type information in the map and new team members would never know that to expect from this model unless they look at the raw JSON responses.

You should always define different types for different responses. AutoValue will be your best friend if you’re still stuck with Java.

extras

A ton of extra values in one bundle:

1
2
3
4
5
6
7
public void onCreate(Bundle bundle) {
    bundle.getString(KEY_1);
    bundle.getString(KEY_2);
    // more omitted 
    bundle.getInt(KEY_10);
    bundle.getString(KEY_11);
}

I know it can be tedious to write Parcelable implementation, but it is also tedious to read and write 10 different properties to a bundle object. You have to create 10 different keys and you also have to know and check the nullability of each property. It will be much easier to maintain if they are wrapped into one predefined object.

Thoughts

Often we want our codebase to be concise, but we should always check if we are trading the readability and maintainability with less code. Being explicit is not always equal to verbose. It can be more code but less error-prone.