Getting rid of an inordinate use of the `instanceof` function

Question

I am working with a library that has a base class Base, and three class descending from it Destination, Amenity and Landmarks. The Base class looks something like this

public class Base {
 public Integer id;
 public String externalId;
 public Base() {
 }
 public int getId() {
 return this.id;
 }
 public String getExternalId() {
 return this.externalId != null ? this.externalId : "";
 }
}

The children classes look like this

public class Destination extends Base implements Parcelable {
 private String name;
 private String description;
 private Point[] points
 public String getName() {
 return this.name;
 }
 public String getDescription() {
 return this.description;
 }
 public Point[] getPoints(){
 return this.points;
 }
}

The important thing to note here is that the three child classes have the points property and the getPoints getter for accessing it. If it was my implementation, I would bring the points field into the base class to avoid replicating multiple times in the child classes but like I said, it's a library and I cannot modify the source. While working with the library I have ran into a situation where I have had to use the instanceof function multiple times and since this is considered a code smell, I'd like to know if there is a better way of going about it. The situation I'm describing happens when I try to get one of the child objects when a point item is specidied, the code looks like this right now

public static <T> T getCurrentMapObjectWithPoint(Point point, T [] bases){
 T base = null;
 for(T model: bases){
 Point[] points = getpointsArrayFromMapObject(model);
 for(Point currentpoint: points){
 if(currentpoint.id.equals(point.id)){
 return base;
 }
 }
 }
 return base;
 }
 private static <T> point[] getpointsArrayFromMapObject(T base){
 if(base instanceof Amenity ){
 return ((Amenity) base).getpoints();
 } else if(base instanceof Destination){
 return ((Destination) base).getpoints();
 } else if (base instanceof LandMark){
 return ((LandMark) base).getpoints();
 }
 return new point[]{};
 }

So simply put, given a Point and list of map items(T[] base - which means and array of either Amenity, Destination or Landmark) I'm trying to figure out which Amenity, Destination or LandMark the point belongs to which means I have to go throgh the list of map items and for each item, go through the points for that item and whenever the point id is equal to the id of the point given at the start I break out of the loop and return the current map item. Is there a better way of doing this without using instanceof multiple times?

• drekbour: Are these classes instantiated by your code (i.e. new Amenity()) or do they materialise out of the library?

Answer 1

I will assume that you really, really want to use the classes defined in the library and not make your own. Presumably the library classes offer other benefits that were not otherwise relevant to the question.

One thought is that you could define an interface that offers a function with the same signature as getPoints, though with a different function name.

public interface PointListGettable {
 public Point[] getPointList();
}

(You'd probably want to come up with a better name than PointListGettable, however.)

Extend the offending classes so they implement this interface. For example:

public MyDestination extends Destination implements PointListGettable {
 public Point[] getPointList() {
 return getPoints();
 }
}

Now you can implement a function with a signature like

public PointListGettable getCurrentMapObjectWithPoint(Point point, PointListGettable [] bases)

but rather than define a function like your getpointsArrayFromMapObject, within the body of the for(PointListGettable model: bases) loop you can just write model.getPointList().

If it's just a matter of getting around one perceived defect in the inheritance scheme without using instanceof, however, I'm not sure all this is worth it.

Answer 2

Create a class that extends Base that has the points field and have your 3 classes extend that class.

Your improved method would look like this: (Note: you should give it a better name)

private static <T> point[] getpointsArrayFromMapObject(T base){
 if(base instanceof MyBase ){
 return ((MyBase) base).getpoints();
 }
 return new point[]{};
}

However It looks like these methods only relate to the type Base. If so you can change your generic to extend from it:

private static <T extends MyBase> point[] getpointsArrayFromMapObject(T base) 
{
 return base.getpoints();
}

You should note it does not matter if each class implements its own getPoints method. The correct method will be executed based on the type of Object passed. This is known as polymorphism.

At which point, there is no need for an additional method:

public static <T extends Base> T getCurrentMapObjectWithPoint(Point point, T [] bases){
 T base = null;
 for(T model: bases){
 Point[] points = base.getPoints();
 for(Point currentpoint: points){
 if(currentpoint.id.equals(point.id)){
 return base;
 }
 }
 }
 return base;
}

• Oyebisi: apologies but the children classes are also in the library and cannot be modified by me to extend a new class.

Answer 3

You cannot avoid some kind of instanceof checks but you can do them once only. This is useful if you intend to check the same objects multiple times. It seems like a lot of effort but I've had to deal with co-mingled types a few times over the years so it's a useful pattern to know.

Example 1 - group by Class

Here we map a stream of unrelated classes into something that can be operated on by type-specific code. There is some ugly (but semantically safe) type-erasure going on in there.

 @SuppressWarnings("unchecked")
 private static Object findByPoint(String target, Map<Class, List> mapped ) {
 for( String s : (List<String>)mapped.get(String.class) ) {
 if ( s.equals(target)) return s;
 }
 for( Long l : (List<Long>)mapped.get(Long.class) ) {
 if ( l.toString().equals(target)) return l;
 }
 for( Float f : (List<Float>)mapped.get(Float.class) ) {
 if ( f.toString().equals(target)) return f;
 }
 return null;
 }
 public static void main(String[] args) {
 Object[] things = {"", 1L, 2f};
 Map<Class, List> mapped = (Map)Arrays.stream(things)
 .collect(groupingBy(Object::getClass));
 Object o = findByPoint("1", mapped);
 }

Example 2 - group by Enum

If you want something more "runtime optimisable" then separate the conditional code into discrete classes. Enums and EnumMap are good for this.

 @SuppressWarnings( {"unchecked", "unused"})
 private enum BaseType {
 StringType() {
 @Override
 String findByPoint(String target, List items) {
 for (String s : (List<String>)items) {
 if (s.equals(target)) {
 return s;
 }
 }
 return null;
 }
 },
 LongType() {
 @Override
 Long findByPoint(String target, List items) {
 for (Long l : (List<Long>)items) {
 if (l.toString().equals(target)) {
 return l;
 }
 }
 return null;
 }
 },
 FloatType() {
 @Override
 Float findByPoint(String target, List items) {
 for (Float f : (List<Float>)items) {
 if (f.toString().equals(target)) {
 return f;
 }
 }
 return null;
 }
 };
 abstract <T> T findByPoint(String target, List items);
 public static BaseType typeForObject(Object o) {
 return BaseType.valueOf(o.getClass().getSimpleName() + "Type"); // TODO Neither clever nor safe
 }
 }
 private static Object findByPoint(String target, Map<BaseType, List<Object>> mapped) {
 Object o;
 for (Map.Entry<BaseType, List<Object>> entry : mapped.entrySet()) {
 if ((o = entry.getKey().findByPoint(target, entry.getValue())) != null) {
 return o;
 }
 }
 return null;
 }
 public static void main(String[] args) {
 Object[] things = {"", 1L, 2f};
 Map<BaseType, List<Object>> mapped = Arrays.stream(things)
 .collect(groupingBy(BaseType::typeForObject, () -> new EnumMap<>(BaseType.class), toList()));
 Object o = findByPoint("1", mapped);
 }
````