1

In the code below, the type parameter D can be either a List<Byte> or a List<List<Byte>> (it is the third generic parameter in the Fields<?, ?, D> interface but still I might omit it there - but it is present also in the return type of the method). Can't seem to find a way to tell the compiler this - get Unchecked cast warnings in the lines marked //* :

public static <D, K, T extends Enum<T> & Fields<?, ?, D>> List<EnumMap<T, D>> 
        getEntries(InputStream is, Class<T> fields) throws IOException {
    final List<List<Byte>> entries = new ArrayList<List<Byte>>();
    // populate "entries"
    final boolean hasLists = hasLists(fields);
    List<K> daBytes;
    if (hasLists) {
        daBytes = (List<K>) new ArrayList<EnumMap<T, List<List<Byte>>>>(); //*
    } else {
        daBytes = (List<K>) new ArrayList<EnumMap<T, List<Byte>>>(); //*
    }
    final int numOfEntries = entries.size();
    for (int currentEntry = 0; currentEntry < numOfEntries; ++currentEntry) {
        // add an element in daBytes for this currentEntry
        if (hasLists) {
            daBytes.add((K) new EnumMap<T, List<List<Byte>>>(fields)); //*
        } else {
            daBytes.add((K) new EnumMap<T, List<Byte>>(fields)); //*
        }
        for (T daField : fields.getEnumConstants()) {
            List<Byte> field = new ArrayList<Byte>();
            // populate "field"
            D map = (D) daBytes.get(currentEntry);
            if (hasLists) {
                List<List<Byte>> fieldEntries = new ArrayList<List<Byte>>();
                // populate "fieldEntries"
                ((EnumMap<T, List<List<Byte>>>) map).put(daField,
                    fieldEntries); //*
            } else {
                ((EnumMap<T, List<Byte>>) map).put(daField, field); //*
            }
        }
    }
    return (List<EnumMap<T, D>>) daBytes; //*
}

If hasLists is false then I need D to be a List<Byte> else a List<List<Byte>>. The daList variable is a List<EnumMap<T, D>>. Now it would seem natural (to me) to define :

List<EnumMap<T, D>> daBytes;

But as soon as I do this and change :

if (hasLists) {
    daBytes = (List<EnumMap<T, D>>) new ArrayList<EnumMap<T, List<List<Byte>>>>();
}

I get an error :

Cannot cast from ArrayList<EnumMap<T,List<List<Byte>>>> to List<EnumMap<T,D>>

Been going round in circles making daBytes an Object, a List<?> etc but always getting to use either outright casts or generic casts that lead to warnings. There must a way to have this compile cleanly with no casts

Mr_and_Mrs_D
  • 32,208
  • 39
  • 178
  • 361
  • 12
    Please consider refactoring, heavily. – Sotirios Delimanolis Sep 11 '13 at 19:15
  • @LuiggiMendoza : your edit has the quite unhelpful result of having java- prepended at the page title - this is one of the bad ideas in SO : http://meta.stackexchange.com/questions/160403/remove-tags-from-page-titles – Mr_and_Mrs_D Sep 11 '13 at 19:27
  • I do not understand what you meant in the comment to my question title edit. – Luiggi Mendoza Sep 11 '13 at 19:30
  • @LuiggiMendoza: see the link - especially the pic :) – Mr_and_Mrs_D Sep 11 '13 at 19:30
  • 1
    Yes I know that. If you think that's a problem, post a question in meta. – Luiggi Mendoza Sep 11 '13 at 19:31
  • 2
    Agree that refactoring is indicated here. This seems to be trying to do things with generics that would be better accomplished with some method overloads that ultimately all call the same single method. You may want to read the excellent [generics tutorial](http://www.cs.rice.edu/~cork/312/Readings/GenericsTutorial.pdf) to understand why casts to generic types are not safe. – VGR Sep 11 '13 at 19:45
  • @VGR: well, I disagree - the nature of the codebase does not leave much space - and anyway my question is about how to make this work - see it as a puzzle - you are welcome to post an answer with methods overloads if you want but generic comments about refactoring are not constructive are they ? :) – Mr_and_Mrs_D Sep 11 '13 at 19:51
  • @LuiggiMendoza: I linked to one such question (with a duplicate) – Mr_and_Mrs_D Sep 11 '13 at 19:56
  • The top comment is a suggestion that you should refactor heavily. I entirely agree and I suggest that you use generics a lot less. – toto2 Sep 11 '13 at 20:05
  • @Mr_and_Mrs_D A comment pointing out a horrible code smell is fairly constructive. I recommend either relying heavily on casts, or on generics, but not mixing the two. – Paul Bellora Sep 11 '13 at 20:46
  • The best thing you can do is simple delegation: Create a wrapper class (or interface) with two subclasses (or implementing classes), one which deals with `List` and one which deals with `List>`. It is not possible to represent both with a single generic type, because they have nothing in common except both being Lists. – VGR Sep 11 '13 at 22:37
  • @PaulBellora: I know it is a code smell that's why I posted it here - my objective is to get rid of the casts (apart maybe one) and rely on generics entirely - the question boils down to the inability to cast `from ArrayList>>> to List>` which I hope there is some way to workaround using some bound on D indicating that it is a `List` of something (`>` won't work cause it is "invariant") – Mr_and_Mrs_D Sep 12 '13 at 08:35

1 Answers1

2

I did a bit of refactoring, extracting a "method object" as suggested by other posters. This is an instance of the so called "strategy class" design pattern.

Wherever you have a check on hasLists I introduced an abstract method. It turns out that you do not need the K type parameter anymore.

The code produces one unchecked warning at the top, where I have put a check on hasLists to choose an implementation of the abstract class.

public static <X, T extends Enum<T> & Fields<?, ?, List<X>>>
List<EnumMap<T, List<X>>> getEntries(InputStream is, Class<T> fields) throws IOException {
    final List<List<Byte>> entries = new ArrayList<List<Byte>>();
    // populate "entries"

    FieldsStrategy<X, T> strategy = selectStrategy(fields);
    return strategy.getEntries(entries);
}

private static <X, T extends Enum<T> & Fields<?, ?, List<X>>>
FieldsStrategy<X, T> selectStrategy(Class<T> fields) {
    final boolean hasLists = hasLists(fields);
    return hasLists
            ? (FieldsStrategy<X, T>) new ByteListFieldsStrategy(fields) //* this is the only unchecked warning
            : (FieldsStrategy<X, T>) new ByteFieldsStrategy(fields);    //* this is the only unchecked warning
}

private abstract static class FieldsStrategy<X, T extends Enum<T> & Fields<?, ?, List<X>>> {
    private Class<T> fields;

    public FieldsStrategy(Class<T> fields) {
        this.fields = fields;
    }

    public List<EnumMap<T, List<X>>> getEntries(List<List<Byte>> entries) {

        List<EnumMap<T, List<X>>> daBytes = new ArrayList<EnumMap<T, List<X>>>();
        final int numOfEntries = entries.size();
        for (int currentEntry = 0; currentEntry < numOfEntries; ++currentEntry) {
            // add an element in daBytes for this currentEntry
            daBytes.add(new EnumMap<T, List<X>>(fields));
            for (T daField : fields.getEnumConstants()) {
                EnumMap<T, List<X>> map = daBytes.get(currentEntry);
                map.put(daField, getFieldData(daField));
            }
        }
        return daBytes;
    }

    protected abstract List<X> getFieldData(T daField);

}

public static class ByteFieldsStrategy<T extends Enum<T> & Fields<?, ?, List<Byte>>>
        extends FieldsStrategy<Byte, T> {
    public ByteFieldsStrategy(Class<T> fields) {
        super(fields);
    }

    protected List<Byte> getFieldData(T daField) {
        ArrayList<Byte> field = new ArrayList<Byte>();
        // populate "field"
        return field;
    }
}

public static class ByteListFieldsStrategy<T extends Enum<T> & Fields<?, ?, List<List<Byte>>>>
        extends FieldsStrategy<List<Byte>, T> {
    public ByteListFieldsStrategy(Class<T> fields) {
        super(fields);
    }

    protected List<List<Byte>> getFieldData(T daField) {
        List<List<Byte>> fieldEntries = new ArrayList<List<Byte>>();
        // populate "fieldEntries"
        return fieldEntries;
    }
}

You probably can remove the sole remaining unchecked warning by moving hasLists() logic and the switch to choose which strategy class to use to the Fields interface and implementing it in the enum classes.

UPDATE: here is the updated definition of selectStrategy and Fields which does not raise any warnings:

public static interface Fields<T extends Enum<T> & Fields<T, D, K>, D extends Data, K> {
    // ....

    GetEntries<K, T> selectStrategy();
}

private static <K, T extends Enum<T> & Fields<T, ?, K>>
GetEntries<K, T> selectStrategy(Class<T> fields) {
    for (T field : fields.getEnumConstants()) {
        return field.selectStrategy();
    }
    throw new IllegalArgumentException("Enum type has no instances: " + fields);
}

You need to implement selectStrategy() in your enum types and return either an appropriate GetByteEntries or GetByteListEntries.

You can remove hasLists() now.

Saintali
  • 4,482
  • 2
  • 29
  • 49
  • Hmm - as it is it produces `GetByte[List]Entries is a raw type. References to generic type C.GetByte[List]Entries should be parameterized` - btw moving `final List> entries = new ArrayList>(); // populate "entries" [here is is used]` in `getEntries()` would dispense of the `is` field in `class GetEntries` – Mr_and_Mrs_D Sep 23 '13 at 10:27
  • Also the `getDaBytes()` and `getEntry()` could be impemented in base class ? As `protected EnumMap getEntry(int currentEntry) { return new EnumMap(fields); }` - no ? – Mr_and_Mrs_D Sep 23 '13 at 19:02
  • I did some cleanup and improvements, including those you suggested. I also extracted a more basic type variable `X` and replaced `D` everywhere with `List`. This latter part should be easy to undo if you don't like it. – Saintali Sep 23 '13 at 20:30
  • I also renamed `GetEntries.invoke()` to `FieldsStrategy.getEntries()` which I feel better reflects the intentions here. Unfortunately it is not possible to fix `selectStrategy()` without more information about `hasLists()`. – Saintali Sep 23 '13 at 20:32
  • I upvoted you as it's only fair since I adopted your solution - will be accepting your answer but before I would appreciate if you had a look at the "haslists" statement in the [actual class](https://github.com/Utumno/MonitoringModel/blob/master/src/gr/uoa/di/monitoring/android/persist/FileStore.java#L502) and see if you can eliminate it somehow. I tried to pin the haslist problem [here](http://stackoverflow.com/a/18858806/281545) with no particular success. Of note : I did not keep the `D` --> `List` - it had occurred to me too but I dropped it (for flexibility ?). Thanks :) – Mr_and_Mrs_D Sep 28 '13 at 09:38
  • 1
    Ah thanks indeed - the problem is that actually this `hasLists()` is a **"static"** property of the _instances_ of the Fields interface. That is a twofold limitation of java : 1. interfaces can't have static methods - which is btw addressed in Java 8 - and 2. static methods can't be overridden - see http://stackoverflow.com/questions/512877/why-cant-i-define-a-static-method-in-a-java-interface – Mr_and_Mrs_D Oct 09 '13 at 19:02
  • Will be implementing your solution and post the link here once I am done - and thanks indeed for following up :) – Mr_and_Mrs_D Oct 09 '13 at 19:03