Are empty abstract classes a bad practice, and why

Question

We have several empty abstract class in our codebase. I find that ugly. But besides this very stupid reason (ugliness), should I refactor it (into empty interface e.g.) ?

Otherwise, the code is robust and well tested. So if it's only for a "aesthetic" reason, I will pass and let the empty abstract classes remain.

What do you think?

EDIT :

1) By "empty abstract class", I mean something like :

public abstract class EmptyAbstractClass {}

2) The reason for the "emptiness" : Hibernate. I don't master this persistence framework at all. I just understand that an interface cannot be mapped to a table, and for this technical reason, a class has been preferred to an interface.

Answer 1

Ask yourself this question: If, as a result of your refactoring, something breaks in production and your continued employment depends on how well you justify your decision to spend time fixing something that wasn't actually broken, what do you say?

"It was ugly and aesthetically offensive to me" isn't an answer I'd like to stake my job on.

At this stage, I say play it safe and live with the Ugly.

Answer 2

It is not necessarily more ugly than the alternative, which may be repeating code.

In an ideal world you would be able to model using only interfaces. for example: Vehicel -> Car -> Pontiac.

But there may be logic which is the same for all Vehicels, so an interface is not appropriate. And you don't have logic specific to Cars. But you do want a Car abstraction because your TrafficLightController wants to distinguish between Cars and Bicycles.

In that case you need to make and abstract Car.

Or you can make an interface Vehicle, a VehicleImpl implements Vehicle, an interface Car extends Vehicle, an interface Pontiac implements Car, and a PontiacImpl implements Pontiac extends VehicleImpl. I personally dislike a parallel hierarchy of interfaces for the sake of preventing an empty abstract class more than an empty abstract class.

So I guess it's a matter of taste.

One caveat; if you use a lot of proxied classes like with Spring and some testing frameworks a parallel interface hierarchy might well result in less unexpected errors.

Answer 3

The obvious questions are, Why was it put there? and How is it used?

My guess is that, either (a) This is the vestige of some false start. An early draft had some data or functions in the abstract class, but as the programmer worked these were all moved elsewhere, until there was nothing left but an empty shell. But more likely (b) At some point there is code that says "if (x instanceof ...". I think this is bad design, basically using a class membership as a flag. If you need a flag, create a flag, don't pretend you're being "more object-oriented" by creating a class or an interface and then using it as a flag. That may fit some textbook definition of better coding but in reality just makes code more confusing.

+1 to Monachus for pointing out that an empty interface is no better than an empty abstract class. Yes, yes, I know Sun did that themselves with Cloneable, but I think that was a lame solution to the problem.

Answer 4

The key is that you can extend from only one abstract class, while you can implement more interfaces.

Apparently the "empty abstract class" design desicion was made so that it prevents the implementing class from extending from another classes.

If it was me I'd let it go, otherwise it might break. Best is still to contact the original developers and ask for reasoning (and add them as comments in the abstract class for your and future convenience).

Answer 5

Sounds to me like this is the result of creating an object heirarchy that ended up not having any common functionality at it's top most levels. I suspect that the immediate subclasses are abstract themselves or at least have subclasses of their own. The other likelyhood is that your code has a lot of instanceof functions scattered throughout it.

The empty topmost level isn't a huge deal in and of itself but I would check to verify that no common functioanlity actually exists. Assuming it does exist I would look at pulling the common features up in the parent classes. I would definately keep a look out for instanceof and think seriously about refactoring it (refer Refactoring to Patterns (Kerievsky) for examples).