Is Hashtable appropriate for storing assets?

Question

I come from an Actionscript3 background and this is my first time writing any Java in my life. Hashtables seem to be similar to Dictionaries in Flash, but I want to make sure I am using them correctly. I believe the Hashtable is typed to accept Strings as keys and Typefaces as objects. Is this correct? Is there a different Collection sub-class that would be more appropriate for something like this? By all means, please tear my n00b Java up. I need to learn this.

package com.typeoneerror.apps.app_name.utils;

import android.content.Context;
import android.graphics.Typeface;

import java.util.Hashtable;

public class FontRegistry
{
    private static FontRegistry _instance;

    private Context                         _context;
    private Hashtable<String, Typeface>     _fonts;

    private FontRegistry()
    {
        _fonts = new Hashtable<String, Typeface>();
    }

    public static FontRegistry getInstance()
    {
        if (_instance == null)
        {
            _instance = new FontRegistry();
        }
        return _instance;
    }

    public void init(Context context)
    {
        _context = context;

    }

    public Typeface getTypeface(int resourceId)
    {
        String fontName = _context.getResources().getString(resourceId);
        if (!_fonts.containsKey(fontName))
        {
            String fontPath = "fonts/" + fontName;
            Typeface typeface = Typeface.createFromAsset(_context.getAssets(), fontPath);
            _fonts.put(fontName, typeface);
        }
        return (Typeface)_fonts.get(fontName);
    }
}

Answer 1

Two suggestions for you.

First, the variable type should refer to the interface of Map. This gives you more flexibility for the future, and will mesh better with most other Java devs.

Second, the implementation should be a HashMap, not a HashTable. HashTable synchronizes everything, where as HashMap does not.

If you do need multithreaded access, I would suggest using a ConcurrentHashMap instead of HashTable. ConcurrentHashMap performs better, as it doesn't lock the entire map during access.

So,

private Map<String, Typeface>     _fonts;

and

_fonts = new HashMap<String, Typeface>();

Lastly, many Java devs would prefer that you don't start member variables with an underscore. Though this is an arguable preference.

EDIT: A last nitpick. You appear to be using a singleton pattern for the registry. This can bite you later, so consider avoiding singletons http://accu.org/index.php/journals/337 . But, ignoring that, you may be better off instantiating the singleton static instance at the declaration. It can avoid a possible contention when fetching it for the first time.

So:

private static FontRegistry _instance = new FontRegistry;

Answer 2

To expand on my comments, your implementation of getInstance() is not thread safe. If you really must use the Singleton pattern, you can use the "Bill Pugh" version (which I have blatantly copied from the wikipedia article):

public class Singleton {

    // Private constructor prevents instantiation from other classes
    private Singleton() {
    }

    /**
     * SingletonHolder is loaded on the first execution of Singleton.getInstance() 
     * or the first access to SingletonHolder.INSTANCE, not before.
     */
    private static class SingletonHolder { 
        public static final Singleton INSTANCE = new Singleton();
    }

    public static Singleton getInstance() {
        return SingletonHolder.INSTANCE;
    }
}

Further you need to be careful when developing for Android not to "leak" a Context. Here is a good article on why doing so is bad, and how to avoid it. The bottom line is that a static reference to a Context (or an object that itself references a Context) can mean that your Activity instance cannot be garbage collected.