Don't Write that Subclass

Inheritance is over-emphasized in object-oriented languages.

Inheritance was conceived as a way of reusing code by structuring it to reflect the real world. Any textbook on the subject will proudly present easy to understand examples: the Circle that subclasses the Shape, the Cat that subclasses the Animal, or the Employee that suclasses the Person. But the reality is that these relationships occur in real world applications much less often than the text would suggest. Nonetheless many programmers are compelled to use inheritance any chance they get, after all, it’s built into the language!

Certainly it’s nice to see an elegant use of inheritance: the subclass overrides the methods of the superclass where their behavior differs, but the subclass still uses methods of the superclass that it need not reimplement. Just as often, however, you’ll see a different pattern: the subclass declares new public methods unrelated to the superclass, but calls methods of the superclass as utilities. It probably looks good to the programmer, hey, it’s code reuse! The problem with this is that the subclass is not really the same type as the superclass in any practical way, and in fact the subclass is never referred to by its supertype at any point in the program. This can be confusing to other maintainers of the application, and also limits the modularity of the subclass.

Common misuses of inheritance

Through my time maintaining and creating object oriented codebases, I’ve run into countless examples of unnecessary uses of inheritance. I’ve categorized the most frequent abuses into three archetypes:

The Initializer

This pattern manifests from the desire to control how specific instances of a class are created. The result, however, is a confusing construct which provides no additional value by being a subclass:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
class DbClient {
private String host;
private int port;

public DbClient(String host, int port) {
this.host = host;
this.port = port;
}

public Result execute(String query) {
//Some code
}
}

class LocalDbClient extends DbClient {
public LocalDbClient() {
this.host = "localhost";
this.port = 1000;
}
}

An easy solution to this is to create a static method on the class or the containing module. This way you can still control how the object is instantiated, but you don’t introduce the additional semantics of a subclass.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
class DbClient {
private String host;
private int port;

public DbClient(String host, int port) {
this.host = host;
this.port = port;
}

public Result execute(String query) {
//Some code
}

public static DbClient createLocalClient() {
return DbClient('localhost', 5432);
}
}

The Method Intersection

Programmers love to use inheritance as a means of code reuse. It’s a conditioned process for many: you write two classes, only to realize that they use many of the same methods in their implementation, and so you create a super class with those common methods, from which they both inherit. Though this is effective in reusing code, it too often results in an ambiguous type hierarchy. What’s more, over time this common superclass will grow with more and more methods that have less and less to do with each other, until the superclass becomes a bloated and unmanageable wreck. Far and away the most common place I see this is in test suites:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
class ApiTest {
boolean assertEquals(Object expected, Object actual){}
boolean assertTrue(Object actual){}
boolean assertNotNull(Object actual){}
boolean getApiClient(){}
...
}

class SomeApiTest extends ApiTest {
@Test
void testApiCall() {
String testUrl = "someurl";
ApiClient client = getApiClient();
Response response = client.makeRequest(testUrl);
assertEquals(response.status, 200);
}
}

At first glance this seems like a valid use of inheritance. SomeApiTest is most certainly a type of ApiTest. But this turns out only to be true in words. Practically speaking, SomeApiTest has no interface in common with ApiTest, it’s simply using the methods of ApiTest as helper methods. The type hierarchy here is meaningless.

The solution here fortunately is straightforward, one can just use a utility class with static methods, or add helpers as members of the test class.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
class Assertions {
static boolean assertEquals(Object expected, Object actual){}
static boolean assertTrue(Object actual){}
static boolean assertNotNull(Object actual){}
...
}

class ClientUtil {
static ApiClient createApiClient(){}
...
}

class SomeApiTest {
@Test
void testApiCall() {
ApiClient client = ClientUtil.createApiClient();
Response response = client.makeRequest();
Assertions.assertEquals(response.status, 200);
}
}

The Helper Mixin

An even more egregious extension of the Method Intersection pattern is the use of “helper mixins” such as those enabled by default methods on Interfaces in Java 8+, or Traits in Scala:

1
2
3
4
5
6
7
8
9
10
11
12
interface ApiHelpers {
default void authenticate(Request request) throws UnauthorizedException {
//Some auth code
}
}

class SomeApiEndpoint implements ApiHelpers {
@GET
public Response get(Request request) {
authenticate(request);
}
}

In this instance, the implements clause is semantically incorrect. SomeApiEndpoint implements none of the methods on the interface in question, and what’s more, the origin of the authenticate method is ambiguous to the reader. This is a clear case where composition or static methods provide for a much more readable implementation:

1
2
3
4
5
6
7
8
9
10
11
12
class ApiHelpers {
static void authenticate(Request request) throws UnauthorizedException {
//Some auth code
}
}

class SomeApiEndpoint {
@GET
public Response get(Request request) {
ApiHelpers.authenticate(request);
}
}

Here the origin of authenticate is clear, and there is no added confusion over additional types introduced by an interface.

Two signs you shouldn’t write that subclass

It’s not always easy to identify when a subclass is the wrong tool, especially when reviewing someone else’s code. I have a couple rules of thumb that I follow when analyzing such a situation.

Generally speaking, you’re misusing inheritance if:

You never refer to the subclass by its superclass

This is a sign that from a type perspective, the two classes have nothing to do with each other.

You’re not overriding (implementing) any methods of the superclass (interface)

This rule follows from the first rule, but is easier to spot. If you’re not overriding any methods, you’re not changing the behavior of the subtype, which means the objects you’re instantiating aren’t really subtypes of the superclass. Similarly if you’re not implementing any methods of an interface, your class doesn’t implement that interface in any meaningful way.


Thanks to Steven Heidel, Dmitrity Afanasyev, and Nick Johnson.


Comments: