0

I get errors with the following code (as described below the snippet):

public class MyClass {
  private Map<String, Subclass1> mapToSubclass1;
  private Map<String, Subclass2> mapToSubclass2;

  public void update(
      final boolean updatesAreSubclass1,
      final List<? extends Superclass> updates) {

    Map<String, Superclass> mapToUpdate;
    if (updatesAreSubclass1) {
      mapToUpdate = mapToSubclass1;
    } else {
      mapToUpdate = mapToSubclass2;
    }


    updates.stream().forEach((entity) -> {
      mapToUpdate.put(entity.getId(), entity);
    });
  }
}

where Subclass1 and Subclass2 extend Superclass, and Superclass provides public String getId();.

As written, I get errors when attempting to define mapToUpdate - Incompatible types. Required: Map<String, foo.bar.Superclass>, Found: Map<String, foo.bar.Subclass1> (or Subclass 2, in the else-clause).

If I change mapToUpdate to a Map<String, ? extends Superclass>, I get an error when attempting to put - Wrong 2nd argument type. Found: 'foo.bar.Superclass', required '? extends foo.bar.Superclass'

I think this is to do with the notion of covariance, but I'm not sure how to resolve the problem. A couple of solutions I came up with, neither satisfactory:

  • Should I need two update methods, one for each Subclass (This gets messy quickly if there are more than two)?
  • Should I move the put inside the if (updatesAreSubclass1) clause, and cast updates to the appropriate List<Subclass>?
scubbo
  • 4,969
  • 7
  • 40
  • 71
  • I dont know if I understood properly, but isn't it okay to just cast the subclass map to mapToUpdate? mapToUpdate = (HashMap) (Map) mapToSubclass1; – Juan Nov 08 '15 at 15:23

5 Answers5

2

Here is a solution that will work with an infinite number of possible subclasses since as far as I can tell you are just creating a map of class plus Id -> Superclass.

private Map<Class,Map<String,Superclass>> map = new HashMap<>();
void update(List<? extends Superclass> l) {
    l.stream().forEach(o -> put(o));
}

public void  put(Superclass obj) {
    String id = obj.getId();
    Map<String,Superclass> submap = map.get(obj.getClass());
    if(null == submap) {
        submap = new HashMap<>();
        map.put(obj.getClass(), submap);
    }
    submap.put(id, obj);
}

public Superclass get(Class clss, String id) {
    return Optional.ofNullable(map)
            .map(m -> m.get(clss))
            .map(m2 -> m2.get(id))
            .orElse(null);
}
WillShackleford
  • 6,918
  • 2
  • 17
  • 33
1

I thing the best way to solved this problem is creating two update methods. One for Subclass1 and one for Subclass2. The reason is simple, better have two single method which do one thing, than one method with boolean parameter which does two things.

This code looks nice and is more testable.

public void update1(final List<Subclass1> updates) {
    updates.stream().forEach((entity) -> {
        mapToSubclass1.put(entity.getId(), entity);
    });
}

public void update2(final List<Subclass2> updates) {
    updates.stream().forEach((entity) -> {
        mapToSubclass2.put(entity.getId(), entity);
    });
}
Mateusz Korwel
  • 1,118
  • 1
  • 8
  • 14
1

Check this

Normal inheritance doesn't work in generics. So, Map<String, Subclass1> doesn't extend from Map<String, SuperClass>.

Your option would be to cast the object explicitly.

if (updatesAreSubclass1) {
    updates.stream().forEach((entity) -> {
      mapToSubclass1.put(entity.getId(), (SubClass1) entity);
    });
} else {
    updates.stream().forEach((entity) -> {
      mapToSubclass2.put(entity.getId(), (SubClass2) entity);
    });
}
Community
  • 1
  • 1
Codebender
  • 14,221
  • 7
  • 48
  • 85
1

Your method does not make a lot of sense with regard to inheritance. You have two separate maps of subclasses and want to add superclass instances in either one of them. I would suggest thinking of a more proper way to deal with this use case.

However, if you want to keep things the way they are, this will do the trick:

public void update(
            final boolean updatesAreSubclass1,
            final List<? extends Superclass> updates) {
  updates.stream().forEach((entity) -> {
    if(updatesAreSubclass1)
      mapToSubclass1.put(entity.getId(), (Subclass1) entity);
    else
      mapToSubclass2.put(entity.getId(), (Subclass2) entity);
    });
}

You cannot store Superclass objects in a map that is defined for a subclass without explicit casting. This should make you think that there may be something wrong with your implementation.

Ivaylo Toskov
  • 3,911
  • 3
  • 32
  • 48
1

You cannot do this:

mapToUpdate = mapToSubclass1;

because then your code could go on to add non Subclass1 objects to mapToUpdate and the compiler wouldn't be able to flag it (i.e., it wouldn't be able to provide type safety).

One way around this is to tell the compiler "I know what I'm doing" and not use generics for your mapToUpdate variable. Like this:

@SuppressWarnings("unchecked")
public void update(final boolean updatesAreSubclass1,
        final List<? extends Superclass> updates) {

    if (updates.size() == 0) {
        return;
    }

    Map mapToUpdate;
    if (updatesAreSubclass1) {
        mapToUpdate = Collections.checkedMap(mapToSubclass1, Integer.class,
                Subclass1.class);
    } else {
        mapToUpdate = Collections.checkedMap(mapToSubclass2, Integer.class,
                Subclass2.class);
    }

    updates.stream().forEach(
            (entity) -> {
                System.out.println("Adding..." + entity.toString()
                        + " to map " + mapToUpdate.toString());
                mapToUpdate.put(entity.getId(), entity);
            });
}

The caveat is that you really do need to know what you are doing, because if you call update with updatesAreSubclass1 = true, when the list is not really a list of Subclass1 objects, you'll get a ClassCastException at runtime.

NOTE: you'll get the ClassCastException because we're using Collections.checkedMap. If you leave that out, you won't get an exception, but you'll get Subclass2 objects in your mapToSubclass1 map -- even worse, right?

Matthew McPeak
  • 17,705
  • 2
  • 27
  • 59
  • This approach would be safer if `update` inspected the list rather than relying on the caller to tell it what type of list it's getting. – Matthew McPeak Nov 08 '15 at 18:02