Does my code smell? Lots of empty, virtual methods

Question

I have a class "Shape" which is a supertype of many other "shapes" in my app.

I have lots of empty methods on the Shape class which are declared as virtual / overridable, so there is not really any default bahaviour. Of course, some sub-shapes implement these methods.

I am really just using it so that I can treat all my shapes as Shape objects.

Is it bad to have these empty methods in my code? Any refactoring possible?

Answer 1

I would not say that this is a code smell but such a class looks a probable candidate to be an Abstract class. It's perfectly fine to add abstractions layers like this.

Answer 2

Isn't it the point of abstract base classes to have no implementation?

The code smell would be if some of these virtual methods were never implemented in the classes below. That would be the sign they are not really needed after all and can be removed.

Answer 3

Modify those methods so that they're abstract ? (Which means that you will have to declare your Shape class as abstract as well)

Then, this means that you will have to override (all) those methods in all your subtypes of Shape. If it seems that this is not necessary in your case , then you will need to refactor, since imho it will mean that your Shape class has to many responsabilities.

But, it is difficult to say without seeing some code.

Answer 4

Unless you specifically mean to imply that there is an empty default behaviour, then you should declare them in the second way (C#):

public abstract Shape {
    // Must be implemented in a subclass
    public abstract SomeMethod();

    // Defined as having no behaviour
    // Doesn't necessarily need to be overriden (possible logic error)
    public virtual SomeOtherMethod() {}
}

The first is probably what you mean to use, instead of the second. If you use the first one, it will force the compiler to throw an error if for example your Square class forgets to override it, while if it forgot to override the second, it would compile, run, and do nothing.

Answer 5

If all you're using the "Shape" class for is to be able to hold all your shapes in a Shape container, or something like that, make it an empty interface. Then, group the methods that "go-together" and place them in their own interfaces. Finally, each individual shape should implement the relevant interfaces.

Answer 6

what about an interface

interface ShapeIF {
     void draw();
}

class Circle implements ShapeIF {
     void draw() {
         /*draws circle */
     }
}