Logo Questions Linux Laravel Mysql Ubuntu Git Menu
 

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

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.
like image 558
depthfirstdesigner Avatar asked Jan 12 '12 20:01

depthfirstdesigner


People also ask

Is it bad practice to use Instanceof?

Probably most of you have already heard that using “instanceof” is a code smell and it is considered as a bad practice. While there is nothing wrong in it and may be required at certain times, but the good design would avoid having to use this keyword.

What can I use instead of Instanceof?

Having a chain of "instanceof" operations is considered a "code smell". The standard answer is "use polymorphism".

What is the use of instance operator?

The instanceof operator in Java is used to check whether an object is an instance of a particular class or not. objectName instanceOf className; Here, if objectName is an instance of className , the operator returns true . Otherwise, it returns false .

Is Instanceof slow Java?

Instanceof is very fast. It boils down to a bytecode that is used for class reference comparison.


1 Answers

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.

like image 145
Tomasz Nurkiewicz Avatar answered Sep 19 '22 17:09

Tomasz Nurkiewicz