Android class in Tic-Tac-Toe

Question

CellDrawableProvider is an Android class used in the game Tic-Tac-Toe. It provides drawables for cells of game board (drawables for figures X and O, and "Fire" if the cell is in the firing line). To save memory, this class uses a map to store the already created drawables.

class CellDrawableProvider {
    private final Map<Object, Drawable> drawables;
    private final Resources resources;

    CellDrawableProvider(Resources resources) {
        this.resources = resources;
        this.drawables = new HashMap<Object, Drawable>();
    }

    Drawable getCellDrawable(int moveIconId, int fireIconId) {
        if (fireIconId == android.R.color.transparent) {
            return getDrawable(moveIconId);
        } else {
            return getLayerDrawable(moveIconId, fireIconId);
        }
    }

    private Drawable getLayerDrawable(int moveIconId, int fireIconId) {
        Object layerDrawableKey = Pair.create(moveIconId, fireIconId);
        Drawable layerDrawable = drawables.get(layerDrawableKey);
        if (layerDrawable == null) {
            layerDrawable = createLayerDrawable(moveIconId, fireIconId);
            drawables.put(layerDrawableKey, layerDrawable);
        }
        return layerDrawable;
    }

    private Drawable createLayerDrawable(int moveIconId, int fireIconId) {
        Drawable[] layers = new Drawable[2];
        layers[0] = getDrawable(moveIconId);
        layers[1] = getDrawable(fireIconId);
        return new LayerDrawable(layers);
    }

    private Drawable getDrawable(int iconId) {
        Drawable drawable = drawables.get(iconId);
        if (drawable == null) {
            drawable = resources.getDrawable(iconId);
            drawables.put(iconId, drawable);
        }
        return drawable;
    }
}

I do not like the class name CellDrawableProvider and method name getCellDrawable().

Can you suggest better names?

Answer 1

You don't need to "cache" the drawables yourself. Android does that internally, as can be seen in the source code.

    Drawable dr = getCachedDrawable(isColorDrawable ? mColorDrawableCache : mDrawableCache, key);

    if (dr != null) {
        return dr;
    }

Therefore, you're actually not saving any memory at all. (Technically you're wasting memory by creating a map for it, but that's so little memory it barely counts)

Instead of creating a LayerDrawable you might want to use setBackgroundResource() and setImageResource() (assuming you're using ImageViews)

As for the name CellDrawableProvider, I don't see what would be wrong with it. It's purpose is clear: A provider for cell drawables.

Answer 2

How about DrawableCellProvider and replacing the function name to 'get()' ,

Also is null really acceptable in

private Drawable getDrawable(int iconId) {
    Drawable drawable = drawables.get(iconId);
    if (drawable == null) {
        drawable = resources.getDrawable(iconId);
        drawables.put(iconId, drawable);
    }
    return drawable;
}

or do you want to return a non-nullable values. Null values tend to bite back and you can spend days looking for the method causing the problem.

Lastly if you really want to be neutoric about code cleanliness and easy to test code, I'd recommend moving the getDrawable() into a DrawableProvider with another get()

Also, If you really want to same memory, why are you trying to put the value every time? I'm assuming the you don't get a new drawable/layerDrawable object for the same key. So doing a check for the key in the map would save you tiny bit of time and ease up your garbage collecction.

would private final Map<Object, Drawable> drawables; be better named as drawableCache. Also, unless two different type of objects can have the same hashcode for very different things, hence I'd recommend having a separate cache for your normal drawables and another one for layerDrawables.

Hope I was more helpful that I currently think I am