Is using .getClass() regarded as a bad design?

Question

I am currently implementing a function, using the superclass as a parameter.

For example:

private void foo(Parent parent) {
    if(parent.getClass() == Child1.class) {
        Child1 c1 = (Child1) parent;
        System.out.println(c1.getChild1Attribute());
    }
    else if(parent.getClass() == Child2.class) {
        Child2 c2 = (Child2) parent;
        System.out.println(c1.getChild2Attribute());
    }
    else if(parent.getClass() == Parent.class) {
        System.out.println(parent.getParentAttribute());
    }
}

Is this a bad idea?

I've read some threads here saying that using getClass() or instanceof is bad design:

It is bad design if you have a better choice. But sometimes you don't. If possible, use the native Java method dispatch instead of the `if-instanceof` idiom. — Marko Topolnik, Apr 23 '13 at 13:50

score 7 · Accepted Answer · edited Apr 23 '13 at 14:00

7

It is not necessarily a bad design, but it is an indication that something wrong may be going on.

Your specific case looks bad, because it appears that a single method is aware of multiple classes. This could be an indication that you are missing an overload possibility, or an opportunity to use one of the multiple dispatch patterns:

// Three overloads - one per target class
private void foo(Parent obj) {
}
private void foo(Child1 obj) {
}
private void foo(Child2 obj) {
}

One of the common multiple dispatch patterns is visitor pattern, see if it is applicable to the problem you're solving.

edited Apr 23 '13 at 14:00

Fred Foo

355,277
75
744
836

answered Apr 23 '13 at 13:51

Sergey Kalinichenko

714,442
84
1,110
1,523

This is just syntactic sugar; the basic design is the same and the knowledge of how `foo` responds to various argument classes is still in a single class. It also requires duplication of the `System.out.println` call. – Fred Foo Apr 23 '13 at 13:54
@larsmans Knowing how to respond to various argument classes is *not* a bad design, as long as you know that you cover them all. Anyone who implemented visitor knows that :):):) – Sergey Kalinichenko Apr 23 '13 at 13:58

score 6 · Answer 2 · answered Apr 23 '13 at 13:50

6

Yes, it is bad design.

Instead, you should make a single abstract method in the superclass and override it in each subclass to perform the desired action.

answered Apr 23 '13 at 13:50

SLaks

868,454
176
1,908
1,964

Fred Foo · Answer 3 · 2013-04-23T13:58:39.777

Yes, this is a sign of bad design. This puts the complexity of handling different classes in a single class instead of encapsulating the relevant knowledge in the appropriate classes themselves. This is going to hurt when you add more classes to your hierarchy, since the compiler won't remind you to implement the relevant new functionality for foo.

A better version would be

private void foo(Parent parent){
    System.out.println(parent.getFooParentAttribute());
}

Then implement getFooParentAttribute on each of the classes.

score 1 · Answer 4 · answered Apr 23 '13 at 13:52

1

Prefer instanceof approach

Josh Bloch on Design

The reason that I favor the instanceof approach is that when you use the getClass approach, you have the restriction that objects are only equal to other objects of the same class, the same run time type.

answered Apr 23 '13 at 13:52

Suresh Atta

120,458
37
198
307

And even then, `instanceof` indicates poor design. (And I am aware you need it for `equals(Object)`.) – Tom Hawtin - tackline Apr 23 '13 at 13:57
@TomHawtin-tackline thanks for correcting me..may i know how ?? – Suresh Atta Apr 23 '13 at 13:58

score 0 · Answer 5 · answered Apr 23 '13 at 13:58

There are two problems with your method as-is. First, suppose that you add a new sub-class of Parent, Child3. This isn't covered by any of your cases, so it prints nothing at all. The same thing is true if you added a new sub-class of Child1, ChildOfChild1. That also wouldn't be covered.

If you were using instanceof instead, then ChildOfChild1 would appear as an instance of Child1 and Child3 would be an instance of Parent.

But generally, the reason that you'd want to avoid this pattern entirely is that all of these cases can be surprising. What's generally better is to write

void foo(Parent p) {...}

and put general code in there, and then for any special cases, create a

void foo(Child1 c) {...}

This makes it clearer what's happening: if you see those two methods, you know that Child1 (and its sub-classes) has some special-casing code, while Parent and any other sub-classes are all treated the same way.

Is using .getClass() regarded as a bad design?

5 Answers5