0

In short, is instanceof a bad thing?

I had a code something like

Converted convert(Object o) {
    if (o instanceof ClassA) {
        convert((ClassA) o);
    }
    if (o instanceof ClassB) {
        convert((ClassB) o);
    }
    throw new IllegalArgumentException(o.class + " not suported");
}

I didn't realize it can be in fact refactored to

Converted convert(Object o) {
    throw new IllegalArgumentException(o.class + " not suported");
}
Converted convert(ClassA a) {
    // do the conversion for ClassA
}
Converted convert(ClassB b) {
    // do the conversion for ClassB
}

ClassA and ClassB are some generated classes without common interface so I cannot do a conversion in one method (without reflection as the method names I'm interested in are the same in ClassA and ClassB).

On the other hand I do not really see a benefit of implementing it that way.

Additionally (when same principe is applied), I'm working with JSF and I have several implementations of javax.faces.convert.Converter, for example

@Component
public class CountryConverter implements Converter {

    @Autowired
    private CountryServiceImpl countryService;

    @Override
    public CountryDto getAsObject(FacesContext context, UIComponent component, String value) {
        return countryService.findById(Long.parseLong(value));
    }

    @Override
    public String getAsString(FacesContext context, UIComponent component, Object value) {
        if (value instanceof CountryDto) {
            Long id = ((CountryDto) value).getId();
            return Long.toString(id);
        }
        return null;
    }

}

...I can have very similarly

    @Override
    public String getAsString(FacesContext context, UIComponent component, Object value) {
        return null;
    }

    @Override
    public String getAsString(FacesContext context, UIComponent component, CountryDto country) {
        return country.getId();
    }

where overloaded version with Object can be in some common parent, but it all seems to me as overengineering = as I mentioned I see no benefit of doing it that way, just because I can. KISS is a principle I like and implementation with instanceof is straightforward...

  • 1
    Possible duplicate of Replacement for instanceof Java? – gnat Jan 9 at 11:21
  • @gnat Thanks, I'll read that for sure, I found also this one before asking - Avoiding instanceof in Java I need some time to think about it still... – Betlista Jan 9 at 11:35
  • Your refactored examples where you put the type into the method arguments are NOT equivalent! Those are method overloads, and the compiler selects one based on the type of variables at compile time (assuming that no reflection is involved). In contrast, instanceof always checks the type of the object at run time. – amon Jan 9 at 22:34
  • Method overloading doesn't work the way you think it does. – immibis Jan 10 at 21:36
7

Using instanceof isn't per se the problem, but it's generally an indicator of a problem where you're explicitly testing an instance's type in order to define behavior. This goes against Liskov's Substitution Principle which states that you should be dealing with an interface or abstract class which can be handled in the same manner throughout your program indifferently of how the concrete class implements it.

Of course I say "generally" because sometimes through the way some libraries such as jsf work, you're given an Object instance, and rather than simply cast it to the class which it should be, you perform a "sanity check" and you test its type anyway. This is to say, use instanceof to check an instance's type because if it isn't this type, we're in a fubar situation here.

If you require your converter class to handle ClassA and ClassB, then unless you can find traits similar to both and create an interface in common with both, you should be creating two separate converter classes, one for ClassA and another for ClassB. It seems like overengineering, but when the alternative is to use instanceof to determine type information to know how to handle it, then I would argue that two single-responsibility classes is simpler than one which awkwardly handles both.

  • Well, but to get an instance of a converted for ClassA/ClassB I have to have somewhere a factory method or something, that would know which converter to use based on type... Or am I missing something? – Betlista Jan 9 at 11:31
  • @Betlista The converter should be configured in your jsf configuration pertaining to a specific type. Simply have a second converter, one to handle ClassA and one to handle ClassB. – Neil Jan 9 at 11:36
  • I have to revisit the idea of shared converter, but I'm afraid it was kind of a shortcut... The JSF converteris clear, on JSF page it is specified, which one to use. Other one (first one) is not on view layer, but service layer... But here my team mates took different approach as technology was not forcing them to have it separated. – Betlista Jan 9 at 11:57
1

instanceof is bad because it forces the user of a set of classes to be aware of all the different subclasses that you invent.

As it is, whenever a new subclass is written, classes that have only a limited involvement with that hierarchy must be changed to match. For systems of any complexity, this is unsustainable.

Dynamic dispatching keeps the knowledge about what subtypes exist to the family of subtypes. A user of a Car doesn't want to write 11 different code paths just because there's ElectricCar and InternalCombustionCar and HybridCar etc. etc. He just wants to obtain a Car from a factory and have car.start() do the right thing without having to know what exact type of car he has. Type-specific overriding within the subclasses achieves exactly that.

  • Well, if I go back to my Converted convert(...) example, it is still aware of all classes... Whether instanceof is used or not... – Betlista Jan 9 at 11:27

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.