Test Driving away Coupling in Activities

Activitys and Fragments, perhaps by some strange historical accidents, have been seen as the optimal building blocks upon which we can build our Android applications for much of the time that Android has been around. Let’s call this idea – the idea that Activitys and Fragments are the best building blocks for our apps – “android-centric” architecture.

This series of posts is about the connection between the testability of android-centric architecture and the other problems that are now leading Android developers to reject it; it’s about how our unit tests are trying to tell us that Activitys and Fragments – like the cracking bricks in the above image – don’t make the best building blocks for our apps because they tempt us to write code with tight coupling and low cohesion.

Last time, we saw Activitys and Fragments tend to have low cohesion. This time, we’ll see how our tests can tell us that code within Activitys have tight coupling. We’ll also see how test driving the functionality leads to a design that has looser coupling, which makes it easier to change the app and also opens up opportunities for removing duplication. As with the the other posts in the series, we’ll be discussing all of this using the Google I/O app as an example.

The Target Code

The code that we want to test, the “target code”, does the following: when the user navigates to the map view that shows where all the Google I/O sessions are, it asks for their location. If they reject the permission, we show a toast notifying the user that they’ve disabled an app permission. Here’s a screenshot of this:

Here’s the code that accomplishes this:

@Override
public void onRequestPermissionsResult(final int requestCode,
        @NonNull final String[] permissions,
        @NonNull final int[] grantResults) {

    if (requestCode != REQUEST_LOCATION_PERMISSION) {
        return;
    }

    if (permissions.length == 1 &&
            LOCATION_PERMISSION.equals(permissions[0]) &&
            grantResults[0] == PackageManager.PERMISSION_GRANTED) {
        // Permission has been granted.
        if (mMapFragment != null) {
            mMapFragment.setMyLocationEnabled(true);
        }
    } else {
        // Permission was denied. Display error message.
        Toast.makeText(this, R.string.map_permission_denied,
                Toast.LENGTH_SHORT).show();
    }
    super.onRequestPermissionsResult(requestCode, permissions,
            grantResults);
}

The Test Code

Let’s take a stab at testing this. Here’s what that would look like:

@Test
public void showsToastIfPermissionIsRejected()
        throws Exception {
    MapActivity mapActivity = new MapActivity();

    mapActivity.onRequestPermissionsResult(
            MapActivity.REQUEST_LOCATION_PERMISSION,
            new String[]{MapActivity.LOCATION_PERMISSION}, new int[]{
                    PackageManager.PERMISSION_DENIED});

    assertToastDisplayed();
}

Hopefully, you’re wondering what the implementation of assertToastDisplayed() looks like. Here’s the thing: there isn’t a straight forward implementation of that method. In order to implement without refactoring our code, we’d need to use a combination of roboelectric and powermock.

However, since we are trying to listen to our tests and change the way we write code, rather than change the way we write tests, we are going to stop for a moment and think about what this test is trying to tell us:

Our presentation logic that lives inside of MapActivity is tightly coupled with Toast.

This coupling is what drives us to use roboelectric to give us mocked android behavior and powermock to mock the static Toast.makeText method. Instead, let’s listen to our test and remove the coupling.

To guide our refactoring, let’s write our test first. This will ensure that our new classes are loosely coupled. We have to create a new class in this particular case in order to avoid Roboelectric, but ordinarily, we could just refactor already existing classes to reduce coupling.

@Test
public void displaysErrorWhenPermissionRejected() throws Exception {

    OnPermissionResultListener onPermissionResultListener =
            new OnPermissionResultListener(mPermittedView);

    onPermissionResultListener.onPermissionResult(
            MapActivity.REQUEST_LOCATION_PERMISSION,
            new String[]{MapActivity.LOCATION_PERMISSION},
            new int[]{PackageManager.PERMISSION_DENIED});

    verify(mPermittedView).displayPermissionDenied();
}

We’ve introduced a OnPermissionResultListener whose job is just to handle the result of request permission from a user. Here’s the code for that:

void onPermissionResult(final int requestCode,
            final String[] permissions, final int[] grantResults) {
    if (requestCode != MapActivity.REQUEST_LOCATION_PERMISSION) {
        return;
    }

    if (permissions.length == 1 &&
            MapActivity.LOCATION_PERMISSION.equals(permissions[0]) &&
            grantResults[0] == PackageManager.PERMISSION_GRANTED) {
        // Permission has been granted.
        mPermittedView.displayPermittedView();

    } else {
        // Permission was denied. Display error message.
        mPermittedView.displayPermissionDenied();
    }
}

The calls to MapFragment and Toast have been replaced with method calls on the PermittedView, an object that gets passed in through the constructor. PermittedView is an interface:

interface PermittedView {
    void displayPermissionDenied();

    void displayPermittedView();
}

And it gets implemented by the MapActivity:

public class MapActivity extends BaseActivity
        implements SlideableInfoFragment.Callback, MapFragment.Callbacks,
        ActivityCompat.OnRequestPermissionsResultCallback,
        OnPermissionResultListener.PermittedView {
    @Override
    public void displayPermissionDenied() {
        Toast.makeText(MapActivity.this, R.string.map_permission_denied,
                Toast.LENGTH_SHORT).show();
    }
}

This may not the best solution, but it gets us to a point where we can test things. This required that OnPermissionResultListener be loosely coupled with its PermittedView. Loose coupling == definitely an improvement.

Who cares?

At this point, some readers might be skeptical. “Is this definitely an improvement?,” they may wonder to themselves. Here are two reasons why this design is better.

(Neither reason I give, you’ll notice is “the design is better because its testable.” That would be circular reasoning.)

Easier Changes

First, its going to be easier to change this code now that it consists of loosely coupled components, and here’s the kicker: the code that we’ve just tested from the Google I/O app actually did change, and with the tests that we have in place, making those changes will be easier. The code I tested was from an older commit. Later on, the folks working on the I/O app decided to replace the Toast with a Snackbar:

Its a small change, but because we’ve separated OnPermissionResultListener from PermittedView, we can make the change on the MapActivitys implementation of PermittedView without having to think at all about the OnPermissionResultListener.

Here’s what that change would have looked like, using their little PermissionUtils class they wrote for displaying SnackBars.

@Override
public void displayPermissionDenied() {
    PermissionsUtils.displayConditionalPermissionDenialSnackbar(this,
            R.string.map_permission_denied, new String[]{LOCATION_PERMISSION},
            REQUEST_LOCATION_PERMISSION);
}

Again, notice that we can make this change without thinking about the OnPermissionResultListener at all. This is actually exactly what Larry Constantine was talking about when he first defined the concept of coupling back in the 70s:

what we are striving for is loosely coupled systems…in which one can study (or debug, or maintain) any one module without having to know very much about any other modules in the system

–Edward Yourdon and Larry Constantine, Structured Design

Reducing Duplication

Here’s another interesting reason to why the fact that our tests have forced us to remove coupling is a good thing: coupling often leads to duplication. Here’s Kent Beck on this:

Dependency is the key problem in software development at all scales…if dependency is the problem, duplication is the symptom.

-Kent Beck, TDD By Example, pg 7.

If this is true, when we remove coupling, we will often see opportunities to reduce duplication. Indeed, this is precisely what we find in this case. It turns out that there is another classes whose onRequestPermissionsResult is nearly identical to the one in MapActivity: AccountFragment. Our tests drove us to create two classes OnPermissionResultListener and PermittedView that – without much modification – can be reused in these other classes.

Conclusion

So, when we have a hard time testing our Activitys and Fragments, its often because our tests are trying to tell us that our code is tightly coupled. The test’s warning about coupling often come in the form of an inability to make an assertion against the code we’re trying to test.¹

When we listen to our tests, instead of changing them by using Roboelectric our powermock, we’re lead to change in our code in a way that makes it less coupled, which makes it easier to make changes and opens up opportunities to reduce duplication.

Notes

1. It could also show up as an inability to get your target code into the right state for testing. That’s what we saw in this post, for example.