8
\$\begingroup\$

I am trying to implements a map (that can work as a cache) that has strings as keys and values of different types. In my real case once set the key/values in the map will not change.

I used these classes, I take as example the Item 29 on Effective Java 2nd edition.

This is the class for values:

final class CachedValue {

    private Object value;

    public <T> void setValue(Class<T> type, T instance){

        if(type == null){

            log.error("type is null");

            throw new NullPointerException("Type is null");
        }

        value = type.cast(instance);
    }

    public <T> T getValue(Class<T> type){

        return type.cast(value);
    }
}

This for the map that works like a cache:

  public final class Cache {

            private final Map<String, CachedValue> map = Maps.newHashMap();

            public <T> void put(String key, Class<T> type, T value){

                CachedValue cachedValue = new CachedValue();

                cachedValue.setValue(type, value);

                map.put(key, cachedValue);
            }

            public <T> T get(String key, Class<T> type){

                return map.get(key).getValue(type);
            }


            public boolean contains(String key){

                return map.containsKey(key);
            }

      }

Can someone review my code and tell me if it's right or if it can be simplify? I don't need it to be thread-safe.

\$\endgroup\$
7
\$\begingroup\$

No Type Safety

Currently your classes provide no type safety:

Cache cache = new Cache();
cache.put("my key", Integer.class, 5);
// ...
String cached = cache.get("my key", String.class);

This compiles but fails at runtime with a ClassCastException thrown by CachedValue.getValue.

What's The Key?

The reason why this can happen is that your class is not clear on the role of the type given to put and get. Considering the internal map, the type is clearly part of the value. But get uses it as it were part of the key. This makes it possible to request a value which is there (considering the string key in the map) but of the wrong type.

Compare this to the typesafe heterogeneous container as defined in Effective Java. There the type is part of the key (actually the only part) so if something is found in the map, it must be of the correct type. It is hence safe to cast to that type.

So in order to make your classes type safe you have to decide on what exactly the key is.

String

If the key is only the string, I see no way to provide type safety. To keep the API from surprising its users you should then simply return Object. This effectively degrades your cache to a map from String to Object, which I guess is not your intention.

String & Class

The other possiblitiy is to make the type a full memeber of the key. There are several ways to implement this and I only present one. It uses nested maps which define this mapping:

String -> (Class -> Object)

This can be interpreted as

(String, Class) -> Object

so the type is part of the key.

public class Cache {

  private final Map<String, Map<Class<?>, Object>> internalMap;

  public Cache() {
    internalMap = new HashMap<>();
  }

  public <T> void put(String textKey, Class<T> typeKey, T chachedValue) {
    Map<Class<?>, Object> mapForTextKey = getMapForTextKey(textKey);
    mapForTextKey.put(typeKey, chachedValue);
  }

  private Map<Class<?>, Object> getMapForTextKey(String textKey) {
    if (!internalMap.containsKey(textKey))
      internalMap.put(textKey, new HashMap<>());
    return internalMap.get(textKey);
  }

  public <T> T get(String textKey, Class<T> typeKey) {
    Object untypedCachedValue = getUntyped(textKey, typeKey);
    // the cast can not fail because:
    // - if there is no value for those keys, the argument is null,
    //      which can always be cast
    // - if there is a value for those keys, `put` made sure that
    //      its type matches the type used as a key
    return typeKey.cast(untypedCachedValue);
  }

  private Object getUntyped(String textKey, Class<?> typeKey) {
    if (internalMap.containsKey(textKey))
      return internalMap.get(textKey).get(typeKey);
    else
      return null;
  }

}

Improvements

My proposal above is only the barebones of an implementation. There is a lot of room for improvements. Some of them are:

  • handling null aftert answering these questions:
    • should it be valid for keys? (please not!)
    • should it be valid for values? (also rather not; doesn't seem to make a lot of sense for a cache)
  • let get return an Optional<T>
  • use Map.computeIfAbsent for more concise code
  • or use Guava's Table instead of the nested maps
  • designing the class for extensibility or making it final

Furhter Comments After The Update

Looks much better. :) These comments are mostly nitpicking...

Comments regarding the code:

  • About null:
    • @NotNull is nice but as far as I know it's purely for tools. To have a runtime guarantee you should add Objects.requireNonNull. On the other hand, you use the HashBasedTable so you're fine - it's documentation says: "Null row keys, columns keys, and values are not supported.". If you rely on that, you should declare table as that type and make sure do write a comment explaining this unusual decision (normally you declare the interface as you did).
    • I'd also make typeKey not-nullable.
    • I like the use of Optional (see here how much). I recommend to use Java 8's version because I expect it to become more common than guava's version.
  • Since Table.getis so concise, getUntyped looks a little pointless now. I would remove the method.
  • I like logging the calls. For symmetry I would also add it to put.

On style:

  • You are very liberal with empty lines. I'd recommend to not do that and only insert an empty line to separate distinct blocks. There seems to be little need for that in the code above.
\$\endgroup\$
  • \$\begingroup\$ I updated my question based on your advices \$\endgroup\$ – res1 Nov 4 '14 at 11:36
  • \$\begingroup\$ I added some new comments based on the update. \$\endgroup\$ – Nicolai Nov 4 '14 at 14:10
  • \$\begingroup\$ Thanks for your comment. About the null check, the annotation I used is @NonNull from projectlombok.org , It ensures that the parameters are not null. About the empty lines I know it's not a good behaviour but I find it more readable for me separating the code by empty lines. \$\endgroup\$ – res1 Nov 4 '14 at 14:35
1
\$\begingroup\$

It looks too complicated and I don't mean the code. I'm not sure how that could be useful in a real scenario.

CachedValue.setValue has a useless cast: value = type.cast(instance). We already know it is of type T.

There is a bug in Cache.get since you might try to access a null pointer with map.get(key).getValue(type).

\$\endgroup\$
  • \$\begingroup\$ I know that about the null pointer exception that can occur, but in my real case i will always call contains() before get() but anyway it's better to put a check there. Why you said that is complicated ? once you created the map you use cache.get(key, clazz) or cache.put(key, clazz, value) \$\endgroup\$ – res1 Nov 2 '14 at 18:08
  • \$\begingroup\$ Return null if map.get(key) is null, otherwise your api is broken. If you have a public method that can trigger a NullPointerException, that's not good. \$\endgroup\$ – toto2 Nov 2 '14 at 18:11
0
\$\begingroup\$

Do not implement it yourself. Use Google Guava ImmutableMap<String, Object>

Take a look at this Guava article about immutable collections.

When I feel need to implement some kind of special collection or cache, I always look first at Google Guava library.

\$\endgroup\$
  • \$\begingroup\$ I was trying to avoid using Object as value type for the map. Using your approach I need to cast the value every time. \$\endgroup\$ – res1 Nov 2 '14 at 22:23
  • \$\begingroup\$ But with your approach, you are doing casting anyway in CachedValue.getValue(). \$\endgroup\$ – luboskrnac Nov 2 '14 at 22:37
  • \$\begingroup\$ Yes but this is in the implementation of the Cache class, a client that use this class doesn't use cast. I understand that it specifies the class calling get/put method but this seems better approach in my opinion. \$\endgroup\$ – res1 Nov 2 '14 at 22:48
  • \$\begingroup\$ User must know the type of the value. Isn't that the same thing as if he would cast it himself? \$\endgroup\$ – luboskrnac Nov 2 '14 at 22:52

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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