10
\$\begingroup\$

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<Bitmap> images;
    // Image Names
    private ArrayList<String> imageNames;
    // Images that need custom scaling
    private ArrayList<String> customScale;

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

    private Images() {
        // init
        images = new ArrayList<Bitmap>();
        imageNames = new ArrayList<String>();
        options = new BitmapFactory.Options();
        customScale = new ArrayList<String>();

        // Assigns image names
        nameImages();

        // Set which images should be custom scaled
        setCustomScales();

        // by default images are null
        for (int i = 0; i < imageNames.size(); i++)
            images.add(null);
    }

    //singleton stuff
    private static class Holder {
        static final Images INSTANCE = new Images();
    }

    public static Images getInstance() {
        return Holder.INSTANCE;
    }

    // recycle all images not in use
    public void recycle(ArrayList<String> imagesInUse, View sv, double scale, DisplayMetrics dm) {
        for (int i = 0; i < imageNames.size(); i++)
            if (!imagesInUse.contains(imageNames.get(i)))
                images.get(i).recycle();

        loadImages(imagesInUse, sv, scale, dm);
    }

    // load all needed images
    public void loadImages(ArrayList<String> imageName, View sv, double scale, DisplayMetrics dm) {
        Context context = sv.getContext();
        for (int ii = 0; ii < imageName.size(); ii++) {
            for (int i = 0; i < imageNames.size(); i++) {
                if (imageNames.get(i).equals(imageName.get(ii))) {
                    int resID = sv.getResources().getIdentifier(imageName.get(ii), "drawable", context.getPackageName());
                    if (customScale.contains(imageName.get(ii))) {
                        images.set(i, BitmapFactory.decodeResource(sv.getResources(), resID));
                        images.set(i, customScaleImage(imageName.get(ii), i, dm));
                    }
                    else {
                        if (scale < .5) {
                            options.inSampleSize = 2;
                            images.set(i, BitmapFactory.decodeResource(sv.getResources(), resID, options));
                            if (scale != .5)
                                images.set(i, Bitmap.createScaledBitmap(images.get(i), (int) (images.get(i).getWidth() * 2 * scale + .5), (int) (images.get(i).getHeight() * 2 * scale + .5), true));
                        } else {
                            images.set(i, BitmapFactory.decodeResource(sv.getResources(), resID));
                            images.set(i, Bitmap.createScaledBitmap(images.get(i), (int) (images.get(i).getWidth() * scale + .5), (int) (images.get(i).getHeight() * scale + .5), true));
                        }
                    }
                }
            }
        }
    }

    // returns bitmap with name of 'imageName'
    public Bitmap getImage(String imageName) {
        for (int i = 0; i < imageNames.size(); i++) {
            if (imageNames.get(i).equals(imageName))
                return images.get(i);
        }
        System.err.println("NO IMAGE OF NAME '" + imageName + "' FOUND!");
        return null;
    }

    // set how you want to custom scale your custom scale images
    private Bitmap customScaleImage(String nameOfImage, int index, DisplayMetrics dm) {
        if (nameOfImage.equals("example"))
            return Bitmap.createScaledBitmap(images.get(index), dm.widthPixels, dm.heightPixels, true);
        System.err.println("CUSTOM SCALE NOT SET FOR " + nameOfImage);
        return null;
    }

    // name all images
    private void nameImages() {
        imageNames.add("example");
    }

    // Add all custom  scale images
    private void setCustomScales(){
        customScale.add("example");
    }
}
\$\endgroup\$
1
  • \$\begingroup\$ can you get away with 2 for loops as that makes the runtime complexity to o(n square) \$\endgroup\$
    – fscore
    Commented May 3, 2014 at 1:44

2 Answers 2

11
\$\begingroup\$

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<Bitmap> 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<String>

instead of

ArrayList<String>

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<Bitmap>();

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.

\$\endgroup\$
1
  • \$\begingroup\$ +1 for everything except Diamond operator. Only API 14+ (if I remember correctly) allows Java 7. Unless you use a plugin like retrolambda \$\endgroup\$ Commented May 3, 2014 at 9:48
6
\$\begingroup\$

Final

private ArrayList<Bitmap> images;

-->

private final List<Bitmap> images;

Whenever you have a variable that doesn't change, mark it as final. In this case the List reference itself doesn't change, so it can be marked as final. All class variables that can be marked as final, should be marked as final.

JavaDoc

You say that this is intended to be used by others, as a Library. However, regular users are used to seeing classes documented with JavaDoc. Please learn how to write JavaDoc.

Math.round

(int) (images.get(i).getWidth() * scale + .5);

What you're doing here is to round the double value to the nearest integer. Use Math.round for that, which would make your intent more clear, instead of adding a seemingly random 0.5 at the end.

Private

Most of your variables are declared as private, which is good, but this is not:

BitmapFactory.Options options;

As far as I can see, that variable can be - and therefore should be - private final.

Android Logging

System.err.println("NO IMAGE OF NAME '" + imageName + "' FOUND!");

The better practice of logging in Android is to use the static methods in the Log class

Log.e("YOUR_TAG", "NO IMAGE OF NAME '" + imageName + "' FOUND!");

This allows users of Logcat to filter on your tag, which greatly simplifies scanning through the endless logging that Android produces.

"example"

I'm sorry but I just cannot see what good these methods do. Why are they there? What's so special about this "example"? Remove it, or come up with a very good reason for why you have it.

// name all images
private void nameImages() {
    imageNames.add("example");
}

// Add all custom  scale images
private void setCustomScales(){
    customScale.add("example");
}

List, or Set?

In your recycle method you have this parameter:

ArrayList<String> imagesInUse

By now you of course know that it should be declared as List rather than ArrayList, but that's not the point here. Let me ask you a question about this variable:

Does the order of the elements matter?

No? Good. Then use Set<String> instead. The .contains method is much more effective on a set than on a List. For more information, see What is the difference between set and list?

You should ask yourself this question for all the times you use a List or a Set. Remember the question: Does the order of the elements matter?

Recycling

What would happen if you'd try to get an image that you have already recycled?

In your recycle method, you don't remove the image from the images list or the imageNames list. I think this will result in unexpected behavior.

Map

In your getImage method you loop through one list to find a matching string, to then return the same index in another list. Rather than doing that, learn how to use a Map<String, Bitmap>. A Map has a lookup time of \$O(1)\$ rather than \$O(n)\$ which you get by looping through the list.

This line should get you started:

Map<String, Bitmap> map = new HashMap<String, Bitmap>();

Look especially on the put, get, containsKey methods in the JavaDoc for the Map interface.

Singleton

First of all I'd question if your class really needs to be a singleton. Even if you would decide that Yes, it should be a singleton, then there's a "better" way of doing it.

public enum SingletonInstance {
    INSTANCE;

    private SingletonObject instance;

    private SingletonInstance() {
        this.instance = new SingletonObject();
    }
}

Usefulness

I'm sorry, but I personally would not use - or recommend - this class to handle images. I feel that it is easier to do handle images directly in my application rather than use your library. In my opinion, your library does not provide functionality, it seems to remove functionality. For example, let's say I'd want to load my image referenced by the id R.drawable.myimage. The way to do that in your library would be to provide it with the string "myimage". If I would rename that image to newimage, your library would not recognize that I have changed the name and therefore cause a problem in runtime, while by using R.drawable.myimage it would either rename itself when I rename the image or I would get a compiler error (which is better than a runtime error).

Don't feel bad about that though, I've coded lots of things that ended up being unneeded. That's how you learn. And besides, that's just my opinion. If you want to use this library yourself, then you've already gained a (hopefully happy) customer.

Keep on coding. And remember that if you want to (which I'd recommend, I hope to be more positive next time), you can ask a follow-up question.

\$\endgroup\$
1
  • \$\begingroup\$ Thanks for the reply. I'm going to fix the nameImage() method so that it adds the image names programmatically rather than having to name them manually. Though I can see that changing an image name would be annoying. \$\endgroup\$
    – java
    Commented May 3, 2014 at 14:29

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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