Is This Use of the "instanceof" Operator Considered Bad Design?

Question

In one of my projects, I have two "data transfer objects" RecordType1 and RecordType2 that inherit from an abstract class of RecordType.

I want both RecordType objects to be processed by the same RecordProcessor class within a "process" method. My first thought was to create a generic process method that delegates to two specific process methods as follows:

public RecordType process(RecordType record){

    if (record instanceof RecordType1)
        return process((RecordType1) record);
    else if (record instanceof RecordType2)
        return process((RecordType2) record);

    throw new IllegalArgumentException(record);
}

public RecordType1 process(RecordType1 record){
    // Specific processing for Record Type 1
}

public RecordType2 process(RecordType2 record){
    // Specific processing for Record Type 2
}

I've read that Scott Meyers writes the following in Effective C++ :

"Anytime you find yourself writing code of the form 'if the object is of type T1, then do something, but if it's of type T2, then do something else,' slap yourself."

If he's correct, clearly I should be slapping myself. I don't really see how this is bad design (unless of course somebody subclasses RecordType and adds in a RecordType3 without adding another line to the generic "Process" method that handles it, thus creating a NPE), and the alternatives I can think of involve putting the brunt of the specific processing logic within the RecordType classes themselves, which really doesn't make much sense to me since there can in theory be many different types of processing I'd like to perform on these records.

Can someone explain why this might be considered bad design and provide some sort of alternative that still gives the responsibility for processing these records to a "Processing" class?

UPDATE:

• Changed return null to throw new IllegalArgumentException(record);
• Just to clarify, there are three reasons a simple RecordType.process() method would not suffice: First, the processing is really too far removed from RecordType to deserve its own method in the RecordType subclasses. Also, there are a whole slew of different types of processing that could theoretically be performed by different processors. Finally, RecordType is designed to be a simple DTO class with minimal state-changing methods defined within.

Maupin

Usually, it's a bad sign. But we need to understand what you're trying to do - and it's not clear from the question. Generally it seems like all your types has the same function, so you can just use interfaces – Pons

What are the difference between RecordType1 and 2? Can you possibly make them adhere to the same interface? – Lanny

You generally don't want to do it, but if you only ever see that logic expressed one time, maybe you live with it. If you start seeing that same if (a instanceof X) logic expressed in multiple places, that's when you really do have to slap yourself. – Jaunty

Hm, do RecordType1 and RecordType2 have any common subclass? – Til

FYI, the specific citation for the Scott Myers quote is from Item 39 "Avoid casts down the inheritance heirarchy" - page 138 of the first edition. He specifically states that it is a good thing that C++ (circa 1992) does not have an instanceof mechanism. – Egression

Answer 1

The Visitor pattern is typically used in such cases. Although the code is a bit more complicated, but after adding a new RecordType subclass you have to implement the logic everywhere, as it won't compile otherwise. With instanceof all over the place it is very easy to miss one or two places.

Example:

public abstract class RecordType {
    public abstract <T> T accept(RecordTypeVisitor<T> visitor);
}

public interface RecordTypeVisitor<T> {
    T visitOne(RecordType1 recordType);
    T visitTwo(RecordType2 recordType);
}

public class RecordType1 extends RecordType {
    public <T> T accept(RecordTypeVisitor<T> visitor) {
        return visitor.visitOne(this);
    }
}

public class RecordType2 extends RecordType {
    public <T> T accept(RecordTypeVisitor<T> visitor) {
        return visitor.visitTwo(this);
    }
}

Usage (note the generic return type):

String result = record.accept(new RecordTypeVisitor<String>() {

    String visitOne(RecordType1 recordType) {
        //processing of RecordType1
        return "Jeden";
    }

    String visitTwo(RecordType2 recordType) {
        //processing of RecordType2
        return "Dwa";
    }

});

Also I would recommend throwing an exception:

throw new IllegalArgumentException(record);

instead of returning null when neither type is found.

Cointon

This seems promising... So what's the chief benefit of using this pattern aside from the fact that it is more complex? Is it simply that one cannot break the framework by trying to extend it and forgetting to include a case? Also where are trzy and cztery (just kidding)? – Maupin

@nomizzz: yes, this pattern enforces you to implement subclass-specific logic at compile time. Also Wikipedia brings another benefit that I wasn't aware of: "visitor object can have state". I should probably redirect you to GoF patterns book instead of quoting it ;-). And yes, the code is a bit more complex and even less readable to some extent. – Cointon

This is a really nice implementation of Visitor. Most people forget the generic type bounds and start losing strong typing. – Mangonel

Instead of visitOne(RecordType1 r) & visitTwo(RecordType2 r), you could use overloading and do visit(RecordType1 r) & visit(RecordType2 r) – Swansea

Answer 2

My suggestion:

public RecordType process(RecordType record){
    return record.process();
}

public class RecordType
{
    public RecordType process()
    {
        return null;
    }
}

public class RecordType1 extends RecordType
{
    @Override
    public RecordType process()
    {
        ...
    }
}

public class RecordType2 extends RecordType
{
    @Override
    public RecordType process()
    {
        ...
    }
}

If the code you need to execute is coupled to something the model shouldn't know (like UI) then you will need to use a kind of double dispatch or visitor pattern.

http://en.wikipedia.org/wiki/Double_dispatch

Diametrically

Yeah as I discussed in my question, I initially considered that solution. There are a few problems with it. First, the processing is really too far removed from RecordType to deserve its own method in the RecordType subclasses. Also, there are a whole slew of different types of processing that could theoretically be performed by different processors. Finally, RecordType is designed to be a simple DTO class with minimal state-changing methods defined within. – Maupin

@ivowiblo Why don't you keep the RecordType abstract as OP suggested and make the process() method abstract as well? – Squilgee

Because I didn't know it's abstract. And as the sample code returns null if it's nor 1 or 2, I left it to the base class. – Diametrically

@nomizzz so, double dispatch is the best. More having in account you are expecting a "whole slew of different types of processing". – Diametrically

Answer 3

Another possible approach would be to make process() (or perhaps call it "doSubclassProcess()" if that clarifies things) abstract (in RecordType), and have the actual implementations in the subclasses. e.g.

class RecordType {
   protected abstract RecordType doSubclassProcess(RecordType rt);

   public process(RecordType rt) {
     // you can do any prelim or common processing here
     // ...

     // now do subclass specific stuff...
     return doSubclassProcess(rt);
   }
}

class RecordType1 extends RecordType {
   protected RecordType1 doSubclassProcess(RecordType RT) {
      // need a cast, but you are pretty sure it is safe here
      RecordType1 rt1 = (RecordType1) rt;
      // now do what you want to rt
      return rt1;
   }
}

Watch out for a couple of typos - think I fixed them all.

Gish

Generally composition/interfaces is better than inheritance, but still a decent fair design proposal. – Lanny

Depends on whether this is a "just once or twice thing", in which case my approach is o.k., or a "this will happen a lot in our code" thing, in which case I definitely agree with you that composition or a Visitor pattern (or similar) would be better. HMM, just saw the OP's clarifications, which indicate the latter - he should use Visitor or an interface. – Gish

Answer 4

Design is a means to an end, and not knowing your goal or constraints, nobody can tell whether your design is good in that particular situation, or how it might be improved.

However, in object oriented design, the standard approach to keep the method implemention in a separate class while still having a separate implementation for each type is the visitor pattern.

PS: In a code review, I'd flag return null, because it might propagate bugs rather than reporting them. Consider:

RecordType processed = process(new RecordType3());

// many hours later, in a different part of the program

processed.getX(); // "Why is this null sometimes??"

Put differently, supposedly unreachable code paths should throw an exception rather than result in undefined behaviour.

Geotaxis

Answer 5

Bad design in one think, as in your example, not using visitor pattern, when applicable.

Another is efficiency. instanceof is quite slow, compared to other techniques, such as comparing against class object using equality.

When using visitor pattern, usually an effective and elegant solution is using Map for mapping between support class and Visitor instance. Large if ... else block with instanceof checks would be very ineffective.

Headpin

Answer 6

It is against open-closed principle of SOLID

Untruth