“Design and document for inheritance or else prohibit it”

The title is the name of Item 17 from the second edition of Joshua Bloch’s Effective Java. I did not see the importance of this advice – or should we call it rule – until I ran into an issue during Android development that was caused by violations of this rule in the Android Framework.

Designing for inheritance is not easy. The aforementioned item in Effective Java writes about the subject with great details. Reading it will reveal the questions and problems that framework developers need to consider if they want to create an easy-to-use, fail-proof API that can be counted on.

While I like Android in general, from a Java developer’s perspective I disagree with some of the design decisions they applied. I will show an example how you can get in trouble because of the framework’s bad design. Please keep in mind that this is only one example, but I’m sure there are many other pitfalls, merely because of how View and its subclasses process their XML attributes in their constructors.

The example

The example will be quite simple. Let’s say that I need a custom switch that can use custom fonts. I’d like to put the font in the assets directory, and set the font on the switch by using an XML attribute. This example can be seen here.

The custom attribute for the font can be seen in the attributes file:

<!-- Attributes for the CustomSwitch widget. -->
<declare-styleable name="CustomSwitch">
    <!-- Path of the font in the assets folder. -->
    <attr name="fontAssetPath" format="string" />
</declare-styleable>

The attribute is processed by CustomSwitch like this:

final TypedArray typedArray = context.obtainStyledAttributes(attrs, R.styleable.CustomSwitch, defStyleAttr, 0);
if (typedArray.hasValue(R.styleable.CustomSwitch_fontAssetPath)) {
    final String fontAssetPath = typedArray.getString(R.styleable.CustomSwitch_fontAssetPath);
    final Typeface typeface = Typeface.createFromAsset(context.getAssets(), fontAssetPath);
    setTypeface(typeface);
}

typedArray.recycle();

In the example project, I decided to use the ON and OFF text for Android. An interesting fact is that SwitchCompat reuses android:textOn and android:textOff attributes, but declares a new one instead of reusing android:showText. Here is the custom switch in the layout:

<com.gyulajuhasz.example.overridefail.CustomSwitch
    android:layout_width="wrap_content"
    android:layout_height="wrap_content"
    android:text="@string/switch_text"
    android:textOff="@string/off"
    android:textOn="@string/on"
    android:thumb="@drawable/custom_switch_thumb"
    app:fontAssetPath="font.ttf"
    app:layout_constraintBottom_toBottomOf="parent"
    app:layout_constraintLeft_toLeftOf="parent"
    app:layout_constraintRight_toRightOf="parent"
    app:layout_constraintTop_toTopOf="parent"
    app:showText="true"
    app:switchPadding="20dp" />

After building and running the app, we can notice an odd thing: the text on the thumb does not use the custom font.

Thumb text is not customized

Let’s fix this! It should be easy, right?

It’s a trap

Examining the API of SwitchCompat, we can easily find the setSwitchTypeface method for changing the typeface of the switch thumb. Unfortunately, there is no XML attribute for specifying this property. The closest we can get to is changing the text appearance of the thumb by using the switchTextAppearance attribute. However, this does not enable changing the typeface to a custom font.

No problem, our style attribute for setting the font is already created, so we can set the thumb’s typeface in the constructor. However, I’d like to ensure that changing the font of the switch always changes the thumb font too. To achieve this, we can use the straight-forward method to override setTypeface(Typeface) and call setSwitchTypeface(Typeface) with the same typeface after calling the superclass’s implementation. This sounds logical and everything will be fine, right?

Actually, it’s quite the opposite.

As soon as you do what I described above, a crash will be your prize. You can see for yourself by building and running the code. The full stack trace can be seen here.

The interesting part:

FATAL EXCEPTION: main
Caused by: java.lang.NullPointerException: Attempt to invoke virtual method 'android.graphics.Typeface android.text.TextPaint.getTypeface()' on a null object reference
   at android.support.v7.widget.SwitchCompat.setSwitchTypeface(SwitchCompat.java:377)
   at com.gyulajuhasz.example.overridefail.CustomSwitch.setTypeface(CustomSwitch.java:38)
   at android.widget.TextView.setTypefaceFromAttrs(TextView.java:1494)
   at android.widget.TextView.(TextView.java:1389)
   at android.widget.Button.(Button.java:109)
   at android.widget.CompoundButton.(CompoundButton.java:84)
   at android.widget.CompoundButton.(CompoundButton.java:80)
   at android.support.v7.widget.SwitchCompat.(SwitchCompat.java:203)
   at com.gyulajuhasz.example.overridefail.CustomSwitch.(CustomSwitch.java:23)
   at com.gyulajuhasz.example.overridefail.CustomSwitch.(CustomSwitch.java:19)

What has happened here?

Looking at the stack trace, it is not too hard to find the root cause. I will quote it from Effective Java:

There are a few more restrictions that a class must obey to allow inheritance. Constructors must not invoke overridable methods, directly or indirectly. If you violate this rule, program failure will result.

And here we have our program failure. Violating the above recommendation – or let’s call it rule – causes errors, because the constructor of the subclass will run only after the superclass constructor. This means when a constructor calls an overridable method, that that method has the potential to operate on a subclass object that does not have its fields initialized yet.

This is exactly just what happens in the custom switch example. The constructor of TextView calls setTypeface(Typeface) when it parses the XML attribute set. When looking at the source of SwitchCompat, it can easily be seen that the private field called mTextPaint is not initialized at this time, since it is initialized only in the constructor of SwitchCompat. Based on these facts, we can easily see that mTextPaint is null when setSwitchTypeface(Typeface) is called from our overridden method.

This StackOverflow answer explains the issue with examples. It’s worth to note one of the comments under this answer written by Helin Wang:

Android engineers be aware: android view’s overridable method invalidate() is sometimes called in view’s constructor.

Unfortunately, it seems that the engineers were not aware, because the Android Framework contains a lot of code like this, especially the View class and its subclasses.

What can be done to fix this?

Framework

First, let’s think this over from the Framework’s perspective. I believe that this is an issue that would be very hard to fix at this point even if the development team had the resources and time for that. Even if they refactored the whole code base to avoid these issues, it would still be wrong on the Android versions that are already released, so it would take years for the faulty versions to disappear.

However, a possible approach would be the separation of creation and initialization. The root of the problem is that the XML attributes are processed in the constructor. Maybe it would be better if this happened in a dedicate life cycle method, just like when Activity initilizes itself its onCreate method. For View, the most suitable method may be onFinishInflate. If every class in the hierarchy initialize themselves in a lifecycle method, it has the following advantages:

all of the constructors have been executed, so proper object creation happened already
every object can choose the moment of executing the superclass implementation
the objects can even skip the superclass implementation, if that’s what they need

In addition to the above, the classes should be more open for extension. For example, looking the source code of the widgets – especially View and TextView -, we see a lot of private and package private fields that could easily be protected in order to make them available for the subclasses. Because of their limited visibility, each and every subclass have to redeclare them when they are needed in that subclass.

Thinking smaller, even if the framework cannot be fixed, individual classes can be improved to reduce the possibility of errors. For example, if the aforementioned mTextPaint field was initialized inline, it may have prevented the crash that occurred because of the override. Following this idea, the chance of this type of failure could be reduced by initializing every fields inline or in initializer blocks where it is possible.

Applications

For application developers, I cannot think of a silver bullet that instantly solves every cases. I believe that whenever a crash or other issue like this occurs, we have to deal with it individually.

For the switch example, the most trivial workaround would be not overriding the method, and call setSwitchTypeface separately in the constructor of CustomSwitch. However, I intentionally chose this way because I did want the thumb text’s font to follow the other text’s font. Given this requirement, here is a terrible workaround that was painful for me to code, but it had to be done. As ugly as it is, it does prevent the crash. The full “fixed” code can be seen here. The above screenshot shows the result. I added a second switch to see the difference, because the chosen custom font is very similar to the default one on the switch thumb. In addition, I added code to log the stack trace of every CustomSwitch.setTypeface(Typeface) calls in order to make the call hierarchy easily trackable by anyone who is interested.

Switch thumb uses custom font too

I’m sure there are other ways to fix this, but this is the best one I could come up with. If you know a more elegant one, you can put it in a comment.

I Help U