Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Casting, instanceof, and @SuppressWarnings("unchecked") are noisy. It would be nice to stuff them down into a method where they won't need to be looked at. CheckedCast.castToMapOf() is an attempt to do that.

castToMapOf() is making some assumptions:

  • (1) The map can't be trusted to be homogeneous
  • (2) Redesigning to avoid need for casting or instanceof is not viable
  • (3) Ensuring type safety in an fail early manner is more important than the performance hit
  • (4) ReturningMap<String,String>is sufficient (rather than returningHashMap<String, String>)
  • (5) The key and value type args are not generic (like HashMap<ArrayList<String>, String>)

(1), (2) and (3) are symptoms of my work environment, beyond my control. (4) and (5) are compromises I've made because I haven't found good ways to overcome them yet.

(4) Is difficult to overcome because even if a HashMap.class was passed into a Class<M> I haven't been able to figure out how to return a M<K, V>. So I return a Map<K, V>.

(5) Is probably an inherent limitation of using Class<T>. I'd love to hear alternative ideas.

Despite those limitations can you see any problems with this java 1.5 code? Am I making any assumptions I haven't identified? Is there a better way to do this? If I'm reinventing the wheel please point me to the wheel. :)

Usage code block:

public class CheckedCast {

    public static final String LS = System.getProperty("line.separator");

    public static void main(String[] args) {

        // -- Raw maps -- //

        Map heterogeneousMap = new HashMap();
        heterogeneousMap.put("Hmm", "Well");
        heterogeneousMap.put(1, 2); 

        Map homogeneousMap = new HashMap();
        homogeneousMap.put("Hmm", "Well");

        // -- Attempts to make generic -- //

        //Unsafe, will fail later when accessing 2nd entry
        @SuppressWarnings("unchecked") //Doesn't check if map contains only Strings
        Map<String, String> simpleCastOfHeteroMap = 
                    (Map<String, String>) heterogeneousMap;  

        //Happens to be safe.  Does nothing to prove claim to be homogeneous.
        @SuppressWarnings("unchecked") //Doesn't check if map contains only Strings
        Map<String, String> simpleCastOfHomoMap = 
                    (Map<String, String>) homogeneousMap;  

        //Succeeds properly after checking each item is an instance of a String
        Map<String, String> checkedCastOfHomoMap = 
                    castToMapOf(String.class, String.class, homogeneousMap);

        //Properly throws ClassCastException
        Map<String, String> checkedCastOfHeteroMap = 
                    castToMapOf(String.class, String.class, heterogeneousMap); 
        //Exception in thread "main" java.lang.ClassCastException: 
        //Expected: java.lang.String
        //Was:      java.lang.Integer
        //Value:    1
        //    at checkedcast.CheckedCast.checkCast(CheckedCast.java:14)
        //    at checkedcast.CheckedCast.castToMapOf(CheckedCast.java:36)
        //    at checkedcast.CheckedCast.main(CheckedCast.java:96)

    }

Methods code block:

    /** Check all contained items are claimed types and fail early if they aren't */
    public static <K, V> Map<K, V> castToMapOf( 
            Class<K> clazzK,    
            Class<V> clazzV,
            Map<?, ?> map) {

        for ( Map.Entry<?, ?> e: map.entrySet() ) {
            checkCast( clazzK, e.getKey() );            
            checkCast( clazzV, e.getValue() );            
        }

        @SuppressWarnings("unchecked")
        Map<K, V> result = (Map<K, V>) map;        
        return result; 
    }

    /** Check if cast would work */
    public static <T> void checkCast(Class<T> clazz, Object obj) {
        if ( !clazz.isInstance(obj) ) {
            throw new ClassCastException(
                LS + "Expected: " + clazz.getName() +
                LS + "Was:      " + obj.getClass().getName() +
                LS + "Value:    " + obj
            );
        }
    }

Some reading I found helpful:

Generic factory with unknown implementation classes

Generic And Parameterized Types

I'm also wondering if a TypeReference / super type tokens might help with (4) and (5) and be a better way to approach this problem. If you think so please post an example.

share|improve this question
3  
"(4) Returning Map<String,String> is sufficient (rather than returning HashMap<String, String>" This is usually expected, we should program to interfaces, not implementations. I will be quite concerned if there are methods expecting a HashMap instead of a Map... –  h.j.k. Sep 2 at 11:55
    
@h.j.k. This is largely true. But I'm trying to make the casting method as ignorant of what's being cast as I can. I hate that it "knows" we're talking about a Map. I've love to get the casting method to where it only knows we're casting some generic "thing" that holds two other "things". Then I could also use it for Pair<T1,T2> and whatnot. –  CandiedOrange Sep 5 at 2:49
    
how do you define a Pair? If it's really just something that holds two other "things", full stop, then you do realize the semantic differences between that and a Map hmms? (we can move this to a chat room if you want to) –  h.j.k. Sep 5 at 5:47
    

1 Answer 1

You should really rewrite the main method as proper unit tests, with separate test cases, for example:

private static void iterateMapKeysAsStrings(Map<String, ?> map) {
    for (String key : map.keySet()) {
        // nothing to do, invalid cast will be triggered for wrong type
    }
}

@Test(expected = ClassCastException.class)
public void testHeterogeneousMap() {
    Map heterogeneousMap = new HashMap();
    heterogeneousMap.put("Hmm", "Well");
    heterogeneousMap.put(1, 2);

    //Unsafe, will fail later when accessing 2nd entry
    //Doesn't check if map contains only Strings
    @SuppressWarnings("unchecked")
    Map<String, String> map = (Map<String, String>) heterogeneousMap;
    iterateMapKeysAsStrings(map);
}

@Test
public void testHomogeneousMap() {
    Map homogeneousMap = new HashMap();
    homogeneousMap.put("Hmm", "Well");

    //Happens to be safe.  Does nothing to prove claim to be homogeneous.
    //Doesn't check if map contains only Strings
    Map<String, String> simpleCastMap = (Map<String, String>) homogeneousMap;
    iterateMapKeysAsStrings(simpleCastMap);

    //Succeeds properly after checking each item is an instance of a String
    Map<String, String> safeCastMap = castToMapOf(String.class, String.class, homogeneousMap);
    iterateMapKeysAsStrings(safeCastMap);
}

I added a helper method iterateMapKeysAsStrings to trigger ClassCastException after an unsafe cast.

For testing an invalid cast with castToMapOf, you don't need to save the result in a variable:

@Test(expected = ClassCastException.class)
public void testInvalidCast() {
    Map heterogeneousMap = new HashMap();
    heterogeneousMap.put("Hmm", "Well");
    heterogeneousMap.put(1, 2);

    //Properly throws ClassCastException
    castToMapOf(String.class, String.class, heterogeneousMap);
}

I couldn't improve the implementation of castToMapOf. I tried a few things, but they didn't work out. The unit tests helped a lot in this: you either get "all green" results, or if something fails, you can pinpoint the problem, without having to read everything including the successful cases.

In comments you wrote that you like the detailed text in the ClassCastException in the checkCast method. I don't really see how this message matters. I understand that if you "test" your code with your original main method, it's easy to read. But with unit tests you don't have to read at all. I think less code is generally better: you could use a shorter message with the LS variable, without the System.getProperty("line.separator").

share|improve this answer
    
I avoided jUnit style so you wouldn't have to scroll down so much. Removing result causes silly compiler errors. I happen to like whitespace. Have anything else to offer? –  CandiedOrange Sep 2 at 10:35
1  
I believe @SuppressWarnings("unchecked") has to be either directly above a single variable or applied to the entire method. –  Simon André Forsberg Sep 2 at 11:48
2  
Yes and it's considered very bad style to apply it to the entire method because it hides other warnings. Despite what my silly eclipse IDE likes to do for me when I click on warnings. –  CandiedOrange Sep 2 at 13:19
    
I removed the incorrect remarks and added some meat in the unit test cases. –  janos Sep 2 at 18:34
    
Is iterateMapKeysAsStrings() just supposed to show off the for loop? Or that you can return void? I don't see that you've changed castToMapOf() at all. –  CandiedOrange Sep 3 at 0:55

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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