1

I'm having some trouble with my inheritance structure for a bunch of wrapper classes with generics. This is basically the structure:

public abstract class SuperWrapper<E extends Super>{
    private E wrappedObject;
    public E getWrappedObject(){
        return wrappedObject;
    }
}

public abstract class MiddleWrapper<E extends Middle> extends SuperWrapper<E>{}

public class SubAWrapper extends MiddleWrapper<SubA>{}

public class SubBWrapper extends MiddleWrapper<SubB>{}

The classes that are wrapped follow the same inheritance structure, so basically:

public class Super{}

public class Middle extends Super{}

public class SubA extends Middle{}

public class SubB extends Middle{}

These wrapped classes are not mine and can not be changed (which is the reason for the wrapper classes). This structure works very well for most purposes, however there are some problems when I want a method to return either a SubAWrapper or a SubBWrapper.

This is what I've tried so far:

public static MiddleWrapper<?> findMiddle(String id){
    SubAWrapper a = new SubAWrapper();
    SubBWrapper b = new SubBWrapper();
    if(returnSubA){
        return a;
    } else{
        return b;
    }
}

This compiles and works but SonarQube warns about the use of a wildcard in a return type and from my googling it's not one of the warnings that should be ignored.

Another way:

public static <E extends Middle> MiddleWrapper<E> findMiddle(String id){
    SubAWrapper a = new SubAWrapper();
    SubBWrapper b = new SubBWrapper ();
    if(returnSubA){
        return (MiddleWrapper<E>) a;
    } else{
        return (MiddleWrapper<E>) b;
    }
}

This compiles and works with the current code, BUT, it seems dangerous. If the caller uses this method like this: MiddleWrapper<SubA> middle = findMiddle("1"); it will compile fine but if findMiddle tries to return a SubBWrapper there will be a ClassCastException at runtime. What I though would work but didn't was:

public static MiddleWrapper<Middle> findMiddle(String id){
    SubAWrapper a = new SubAWrapper ();
    SubBWrapper b = new SubBWrapper ();
    if(returnSubA){
        return (MiddleWrapper<Middle>) a; //Compile error here
    } else{
        return (MiddleWrapper<Middle>) b; //and here
    }
}

So my question is basically, is there a correct way to write the method findMiddle that compiles, runs and follows best practices?

For extra credit, is there a way to write a method so that it returns a list containing both SubA and SubB. Like this but without the wildcard:

public static List<MiddleWrapper<?>> getAllMiddle(){
    List<MiddleWrapper<?>> ret = Lists.newArrayList();
    ret.add(new SubAWrapper());
    ret.add(new SubBWrapper());
    return ret;
}

ps. At this point we're still using Java 6 but an upgrade to Java 8 is scheduled for next year so solutions using Java 7-8 functionality will still be useful eventually.

MatsT
  • 1,759
  • 3
  • 20
  • 33

2 Answers2

1

MiddleWrapper is a generic class parameterized with a Middle class :

public abstract class MiddleWrapper<E extends Middle> extends SuperWrapper<E>{}

If you want to provide a single method that returns a List of MiddleWrapper with at least two distinct generic types for MiddleWrapper, you have no many choices.

If you don't need to set any constraint on the Middle subclass that may be returned.
The method that you shown is fine :

public static List<MiddleWrapper<?>> getAllMiddle() {
  List<MiddleWrapper<?>> ret = Lists.newArrayList();
  ret.add(new SubAWrapper());
  ret.add(new SubBWrapper());
  return ret;
}

You said :

This compiles and works but SonarQube warns about the use of a wildcard in a return type and from my googling it's not one of the warnings that should be ignored.

Why the usage of the ? in MiddleWrapper<?> would be a bad practice ?
The class MiddleWrapper is generic and in your method, you may return any type of Middle used as generic.
It is the way of doing for this use case.

Now, if you need to set a constraint on the Middle subclass that may be returned.
For example, some Middle subclass but not some others.

You could introduce an intermediary class that extends Middle, makes SubA and SubB extend it and type the List with it in the getAllMiddle() method.

public class ValidMiddle extends Middle {...}

public class SubA extends ValidMiddle{...}

public class SubB extends ValidMiddle{...}

And the method should be :

public static List<MiddleWrapper<? extends ValidMiddle>> getAllMiddleWithMoreSpecificType() {
  List<MiddleWrapper<? extends ValidMiddle>> ret = Lists.newArrayList();
  ret.add(new SubAWrapper());
  ret.add(new SubBWrapper());
  return ret;
}

Note that this way is a little more verbose for the client that should manipulate this declared type :

 List<MiddleWrapper<? extends ValidMiddle>> listOfWrappers = getAllMiddle();
davidxxx
  • 125,838
  • 23
  • 214
  • 215
  • The reason it would be bad practice is explained a bit here: https://sonarcloud.io/organizations/default/rules#q=wildcard Also on various stackoverflow questions such as https://stackoverflow.com/questions/22815023/generic-wildcard-types-should-not-be-used-in-return-parameters. If I can use MiddleWrapper> or MiddleWrapper extends Middle> in return types it's easy to solve, the question is if it's possible without it. – MatsT Jul 21 '17 at 12:24
  • To achieve it, MiddleWrapper should be instantiable without specifying any generic. By removing a layer of generic you could. Finally, it is trade off between generic constraint and the easiness of use of the API. – davidxxx Jul 21 '17 at 13:05
1

In example specified in "Another way" paragraph ClassCastException can be avoided by forcing the returned type:

public static <E extends Middle> MiddleWrapper<E> findMiddle(String id, Class<E> clazz){
        SubAWrapper a = new SubAWrapper();
        SubBWrapper b = new SubBWrapper();
        if(returnSubA){
        //check here if a/b is instance of clazz
            return (MiddleWrapper<E>) a;
        } else{
            return (MiddleWrapper<E>) b;
        }
    }

And method call would be:

MiddleWrapper<SubA> middle = findMiddle("1", SubA.class);
krisp
  • 131
  • 5