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 reasons of the "emptyness" : Hibernate. I dont master this persistence framework at all. I just understand that a interface cannot be map to a table, and for this technicall reason a class has bean prefered to an interface.

Answer 1

Interfaces are preferable in this case because it makes your code more robust for maintenance. That is, you can only extend a single class but you may implement many interfaces.

If there is absolutely no direct effect right now I would not touch it. If the maintenance event turns up that requires you to make a change then I would refactor it since I am already in the code.

In other words if it ain't broke don't fix it.

Answer 2

The question to ask is: "What do I want to do with this code that I can't do, or find hard to do, because these empty abstract classes are there?" If the answer is 'nothing', you should leave them alone. If the answer is "something", it may well be worthwhile to remove them - but if you can, speak to the people who created them first, just to make sure there isn't some subtle purpose to them. For example, perhaps your code uses reflection to find all instances of a particular ABC and do special things to them, in which case your refactoring could break the code in subtle ways.

Answer 3

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 4

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 5

Empty abstract classes don't make any sense to me, abstract classes should be used to inherit some behavior. So, I tend to agree, it's a pretty ugly design and a very bad use of abstract classes, marker interfaces should be preferred. So you have two options:

1. Wait till you need to extend another class for a real need and get annoyed by the abstract class to replace it by an interface.
2. Don't wait and fix the design.

In this particular situation, it is true that the actual design doesn't really hurt, for now, so you can live with it. However, I think replacing these abstract classes is a pretty easy refactoring (make them interfaces and replaces extends with implements where you get compilation errors) and I really can't see what could get broken so I would go for it.

Personally, and people may of course disagree, I don't like being too defensive. With rules like if it's ain't broke, don't fix it, you'll never refactor anything ("my tests passes, why should I refactor?"). Freezing the code is definitely not the right solution, testing aggressively is the right solution.

Answer 6

According to object oriented programming theory the main purpose for inheritance is polymorphism, code reuse and encapsulation. An empty abstract class (and when i say this i mean truly empty, no constructors, no methods, no properties. nothing!) does not achieve any of the three goals hoped by this programming technique. It is the equivalent to if(true){...}. changing it to an interface does not really makes it any better.

If you want to refactor the code i would advise you to think in the direction opposite to the one you are thinking, what i mean by this is: try to abstract properties, methods and constructors from all classes that share a abstract parent class.

This is hard work with little reward in the short term but it increases the maintainability of the code dramatically since a core logic change would have to be done only once. I suspect the reason for using those empty abstract classes is to identify a group of classes that must share something in common otherwise what would be the difference between Object and the abstract class

Answer 7

If you have the following pattern you will find it to be the most flexible:

interface Fooable
{
   void foo();
   void bar();
}

abstract class AbstractFoo
    implements Fooable
{
}

class Foo
    extends AbstractFoo
{
   public void foo()
   {
   }

   public void bar()
   {
   }
}

This way you can always pass by the interface but if you later find that you have code that can be common you can put it in the abstract class without having to make changes to all of the classes.

Unless you have a good reason for not doing that pattern (and I suspect you don't in this case) I would suggest using it. This can wind up with empty abstract classes but I think it is ok (a little odd, but ok).

If there truly are no methods in the interface or only one then I would skip the abstract class.

From a compiler/functional point of view there is no real difference between an interface and an abstract class where all method are abstract. The interface will be more flexible than the abstract class, and the interface + abstract class will be the most flexible of all.

If it were my code I'd make the change to them being interfaces...

Answer 8

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 9

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 10

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).