Caching objects built with multiple parameters

I have a factory that creates objects of class MyClass, returning already generated ones when they exist. As I have the creation method (getOrCreateMyClass) taking multiple parameters, which is the best way to use a Map to store and retrieve the objects?

My current solution is the following, but it doesn't sound too clear to me. I use the hashCode method (slightly modified) of class MyClass to build an int based on the parameters of class MyClass, and I use it as the key of the Map.

import java.util.HashMap;
import java.util.Map;

public class MyClassFactory {

    static Map<Integer, MyClass> cache = new HashMap<Integer, MyClass>();

    pr开发者_如何学JAVAivate static class MyClass {
        private String s;
        private int i;

        public MyClass(String s, int i) {
        }

        public static int getHashCode(String s, int i) {
            final int prime = 31;
            int result = 1;
            result = prime * result + i;
            result = prime * result + ((s == null) ? 0 : s.hashCode());
            return result;
        }

        @Override
        public int hashCode() {
            return getHashCode(this.s, this.i);
        }

    }


    public static MyClass getOrCreateMyClass(String s, int i) {
        int hashCode =  MyClass.getHashCode(s, i);
        MyClass a = cache.get(hashCode);
        if (a == null) {
            a = new MyClass(s, i);
             cache.put(hashCode , a);

        } 
        return a;
    }

}

Your getOrCreateMyClass doesn't seem to add to the cache if it creates.

I think this will also not perform correctly when hashcodes collide. Identical hashcodes do not imply equal objects. This could be the source of the bug you mentioned in a comment.

You might consider creating a generic Pair class with actual equals and hashCode methods and using Pair<String, Integer> class as the map key for your cache.

Edit:

The issue of extra memory consumption by storing both a Pair<String, Integer> key and a MyClass value might be best dealt with by making the Pair<String, Integer> into a field of MyClass and thereby having only one reference to this object.

With all of this though, you might have to worry about threading issues that don't seem to be addressed yet, and which could be another source of bugs.

And whether it is actually a good idea at all depends on whether the creation of MyClass is much more expensive than the creation of the map key.

Another Edit:

ColinD's answer is also reasonable (and I've upvoted it), as long as the construction of MyClass is not expensive.

Another approach that might be worth consideration is to use a nested map Map<String, Map<Integer, MyClass>>, which would require a two-stage lookup and complicate the cache updating a bit.

You really shouldn't be using the hashcode as the key in your map. A class's hashcode is not intended to necessarily guarantee that it will not be the same for any two non-equal instances of that class. Indeed, your hashcode method could definitely produce the same hashcode for two non-equal instances. You do need to implement equals on MyClass to check that two instances of MyClass are equal based on the equality of the String and int they contain. I'd also recommend making the s and i fields final to provide a stronger guarantee of the immutability of each MyClass instance if you're going to be using it this way.

Beyond that, I think what you actually want here is an interner.... that is, something to guarantee that you'll only ever store at most 1 instance of a given MyClass in memory at a time. The correct solution to this is a Map<MyClass, MyClass>... more specifically a ConcurrentMap<MyClass, MyClass> if there's any chance of getOrCreateMyClass being called from multiple threads. Now, you do need to create a new instance of MyClass in order to check the cache when using this approach, but that's inevitable really... and it's not a big deal because MyClass is easy to create.

Guava has something that does all the work for you here: its Interner interface and corresponding Interners factory/utility class. Here's how you might use it to implement getOrCreateMyClass:

private static final Interner<MyClass> interner = Interners.newStrongInterner();

public static MyClass getOrCreateMyClass(String s, int i) {
  return interner.intern(new MyClass(s, i));
}

Note that using a strong interner will, like your example code, keep each MyClass it holds in memory as long as the interner is in memory, regardless of whether anything else in the program has a reference to a given instance. If you use newWeakInterner instead, when there isn't anything elsewhere in your program using a given MyClass instance, that instance will be eligible for garbage collection, helping you not waste memory with instances you don't need around.

If you choose to do this yourself, you'll want to use a ConcurrentMap cache and use putIfAbsent. You can take a look at the implementation of Guava's strong interner for reference I imagine... the weak reference approach is much more complicated though.