Image handling class

Let me start by saying that I am very happy with this code. I want to share it with other people because I think they will find it helpful, but I want to make sure I did everything in good form before I do so.

The code allows users to easily manage their image memory. The class handles all of your images, which means no image will be unnecessarily created twice. Each time a new class is created the user can just call recycle() to recycle all the images that were used in the previous class and load all the images needed in the class. Images can be added to this custom class by calling loadImages() and the user can redefine and remove items by calling recycle() with only the images they want to keep (they can also add image here). They can also define images to be scaled custom.

All of that can be done by properly using the onResume and OnPause methods, but this class greatly simplifies it and neatens the code in my opinion. BUT this class can do something that can't be done with the onPause and onResume methods. When loading a new classes images, the images used in the previous classes don't need to be reloaded. Depending on the application the time this saves varies. In my application it saves a visible amount of time due to the large number of duplicate images used in different classes.

What do you think? How could this be improve? Is it worth sharing?

```
import java.util.ArrayList;

import android.content.Context;
import android.graphics.Bitmap;
import android.graphics.BitmapFactory;
import android.util.DisplayMetrics;
import android.view.View;

public class Images {

// Image Holder
private ArrayList images;
// Image Names
private ArrayList imageNames;
// Images that need custom scaling
private ArrayList customScale;

// Options, used for proper scaling
BitmapFactory.Options options;

private Images() {
// init
images = new ArrayList();
imageNames = new ArrayList();

Glad to hear that you're looking for a review before publishing it! Let's get right to it.

Class naming

Your class is named Images but that doesn't convey a meaning to me. In fact, it's a plural so I would never use this for a class name (how would you describe multiple instances of this class?).

Image by itself is obviously not a good name either, since the class does not represent that. I have only vaguely looked through the actual class and its purpose yet but something like ImageManager seems like a good fit here.

Note that the use of *Manager is usually a sign of the anemic domain model but this is one of the cases where that doesn't apply: you actually are managing a collection of images, things that can't be done by the images themselves. You can work with the context for a different name (for example ImageLibrary), but that depends on how you want this to be viewed, really.

Comments

Comments should explain why you do something, not what you do. The latter should be done through good naming and adhering to conventions for readability.

Concretely this means that this is redundant:

// Image Holder
private ArrayList images;

I already know it stores images in a collection, I can see that by reading the code (which is what I read before I read comments).

Likewise here (and many other places, these are just a few examples:

// Assigns image names
nameImages();

Working towards an interface

I will refer you to this SO post on this subject.

What this exactly means for you is that you should define your fields as

List

instead of

ArrayList

Doing so will allow you to change it up to a LinkedList (if you'd want to) without breaking any contracts in your code.

Diamond operator

Since Java 7 there is the so called diamond operator available to us. This allows us to omit the type parameter in a generic instantiation.

This would turn

new ArrayList();

into

new ArrayList<>();

Brackets

Always add brackets to your if, for, while, etc statements. If you don't, you will undoubtedly run into a logical error eventually when you decide you want your if statement to perform 2 actions instead of just that 1.

Indentation

Right now your method bodies are indented with 2 tabs, conventions state that one tab should be used consisting of 4 spaces (half you have right now). Which makes sense because reading your code definitely makes me noticed the whitespace. Nested blocks will also fill up your screen very quickly like this.

Descriptive names

For a single loop I'll usually use i as variable as well but when you have deeply nested loops like loadImages then you should really use a name with more meaning. The code is filled with references to i and ii and I have no idea what they resemble.

Likewise you should make the names say as much as they can on their own: dm and resID are unnecessary abbreviations, displayMetrics and resourceID are much more pleasant to read.

Intermediate values

In this same loop you have the following expression:

images.set(i, Bitmap.createScaledBitmap(images.get(i), (int) (images.get(i).getWidth() * scale + .5), (int) (images.get(i).getHeight() * scale + .5), true));

To me, it is very hard to take away what this is about. If you would instead use intermediate variables it would be a lot more readable (and easier to debug if something is wrong!). This could become:

Bitmap sourceImage = images.get(i);
int newWidth = (int) (images.get(i).getWidth() * scale + .5);
int newHeight = (int) (images.get(i).getHeight() * scale + .5);
boolean filter = true;

images.set(i, Bitmap.createScaledBitmap(sourceImage, newWidth, newHeight, filter);

Error handling

It is very important that your methods do the thing they're described to do and nothing more. Looking at the getImage method I see a call to System.err.println, which I could never have guessed from the method's name.

Instead use proper error handling by providing an actual response. This response can manifest itself as a return value or an exception. Returning null (for reference type return values) has had its fair share of discussion so I'll let you read through it.

Generally I will always try to avoid null as a return value but that doesn't mean it is always inappropriate to use. For collections it's easy: return an empty collection. For a single object there is more room for preference.

Here I would use an exception though, InvalidArgumentException is the most appropriate. Attempting to retrieve a non-existing image is probably a truly exceptional action and should thus be treated as one.

Method naming

Methods should describe actions. When I see a method customScaleImage I have no idea what it will do. Looking at the implementation tells me it will only return something so getCustomScaleImage is appropriate.

StackExchange Code Review Q#48830, answer score: 11