5
\$\begingroup\$

Retrieving an instance of (Default) SharedPreferences and then calling the six methods that may be called to put/get a value gets pretty old pretty quickly, so I wrote this:

import android.annotation.SuppressLint;
import android.annotation.TargetApi;
import android.content.Context;
import android.content.SharedPreferences;
import android.content.SharedPreferences.Editor;
import android.os.Build;
import android.preference.PreferenceManager;

import java.util.ArrayList;
import java.util.List;
import java.util.Set;

public final class AccessPreferences {

    private static final List<Class<?>> CLASSES = new ArrayList<Class<?>>();
    static {
        CLASSES.add(String.class);
        CLASSES.add(Boolean.class);
        CLASSES.add(Integer.class);
        CLASSES.add(Long.class);
        CLASSES.add(Float.class);
        CLASSES.add(Set.class);
    }

    private AccessPreferences() {}

    private static SharedPreferences prefs;

    // TODO: do I need synchronized ? (1)
    private static synchronized SharedPreferences getPrefs(Context ctx) {
        if (prefs == null) {
            prefs = PreferenceManager.getDefaultSharedPreferences(ctx);
        }
        return prefs;
    }

    @TargetApi(Build.VERSION_CODES.HONEYCOMB) // (5)
    public static <T> void persist(Context ctx, String key, T value) {
        @SuppressLint("CommitPrefEdits")
        final Editor ed = getPrefs(ctx).edit();
        if (value == null) {
            // commit it as that is exactly what the API does - can be retrieved
            // as anything but if you give get() a default non null value it
            // will give this default value back
            ed.putString(key, null);
        } else if (value instanceof String) ed.putString(key, (String) value);
        else if (value instanceof Boolean) ed.putBoolean(key, (Boolean) value);
        // TODO : IS THE ORDER OF FLOAT, INTEGER AND LONG CORRECT ?  // (4)
        else if (value instanceof Integer) ed.putInt(key, (Integer) value);
        else if (value instanceof Long) ed.putLong(key, (Long) value);
        else if (value instanceof Float) ed.putFloat(key, (Float) value);
        else if (value instanceof Set) {
            if (android.os.Build.VERSION.SDK_INT < Build.VERSION_CODES.HONEYCOMB) {
                throw new IllegalStateException(
                        "You can add sets in the preferences only after API "
                            + Build.VERSION_CODES.HONEYCOMB);
            } // (5)
            // The given set does not contain strings only --> TODO : not my
            // problem ? probably cause the one who filled it made the mistake (3)
            // Set<?> set = (Set<?>) value;
            // if (!set.isEmpty()) {
            // for (Object object : set) {
            // if (!(object instanceof String))
            // throw new IllegalArgumentException(
            // "The given set does not contain strings only");
            // }
            // }
            @SuppressWarnings({ "unchecked", "unused" })
            Editor soIcanAddSuppress = ed
                    .putStringSet(key, (Set<String>) value);
        } else throw new IllegalArgumentException("The given value : " + value
            + " cannot be persisted");
        ed.commit();
    }

    @SuppressWarnings("unchecked")
    @TargetApi(Build.VERSION_CODES.HONEYCOMB) // (5)
    public static <T> T retrieve(Context ctx, String key, T defaultValue) {
        // if the value provided as defaultValue is null I can't get its class
        if (defaultValue == null) {
            // if the key (which can very well be null btw) !exist I return null
            // which is both the default value provided and what Android would
            // do (as in return the default value) (TODO: test)
            if (!getPrefs(ctx).contains(key)) return null;
            // if the key does exist I get the value and..
            final Object value = getPrefs(ctx).getAll().get(key);
            // ..if null I return null
            if (value == null) return null;
            // ..if not null I get the class of the non null value. Problem is
            // that as far as the type system is concerned T is of the type the
            // variable that is to receive the default value is. So :
            // String s = AccessPreferences.retrieve(this, "key", null);
            // if the value stored in "key" is not a String (for instance
            // `"key" --> true` or `"key" --> 1.2`) a ClassCastException will
            // occur _in the assignment_ after retrieve returns
            // TODO : is it my problem ? This :  // (2)
            // SharedPreferences p =
            // PreferenceManager.getDefaultSharedPreferences(ctx);
            // int i = p.getInt(KEY_FOR_STRING, 7);
            // results in a class cast exception as well !
            final Class<?> clazz = value.getClass();
            // TODO : IS THE ORDER OF FLOAT, INTEGER AND LONG CORRECT ?
            for (Class<?> cls : CLASSES) {
                if (clazz.isAssignableFrom(cls)) {
                    try {
                        // I can't directly cast to T as value may be boolean
                        // for instance
                        return (T) clazz.cast(value);
                    } catch (ClassCastException e) { // won't work see :
                        // https://stackoverflow.com/questions
                        // /186917/how-do-i-catch-classcastexception
                        // basically the (T) clazz.cast(value); line is
                        // translated to (Object) clazz.cast(value); which won't
                        // fail ever - the CCE is thrown in the assignment (T t
                        // =) String s = AccessPreferences.retrieve(this, "key",
                        // null); which is compiled as
                        // (String)AccessPreferences.retrieve(this, "key",
                        // null); and retrieve returns an Integer for instance
                        String msg = "Value : " + value + " stored for key : "
                            + key
                            + " is not assignable to variable of given type.";
                        throw new IllegalStateException(msg, e);
                    }
                }
            }
            // that's really Illegal State I guess
            throw new IllegalStateException("Unknown class for value :\n\t"
                + value + "\nstored in preferences");
        } else if (defaultValue instanceof String) return (T) getPrefs(ctx)
                .getString(key, (String) defaultValue);
        else if (defaultValue instanceof Boolean) return (T) (Boolean) getPrefs(
            ctx).getBoolean(key, (Boolean) defaultValue);
        // TODO : IS THE ORDER OF FLOAT, INTEGER AND LONG CORRECT ?
        else if (defaultValue instanceof Integer) return (T) (Integer) getPrefs(
            ctx).getInt(key, (Integer) defaultValue);
        else if (defaultValue instanceof Long) return (T) (Long) getPrefs(ctx)
                .getLong(key, (Long) defaultValue);
        else if (defaultValue instanceof Float) return (T) (Float) getPrefs(ctx)
                .getFloat(key, (Float) defaultValue);
        else if (defaultValue instanceof Set) {
            if (android.os.Build.VERSION.SDK_INT < Build.VERSION_CODES.HONEYCOMB) {
                throw new IllegalStateException(
                        "You can add sets in the preferences only after API "
                            + Build.VERSION_CODES.HONEYCOMB);
            } // (5)
            // The given set does not contain strings only --> TODO : not my
            // problem ? probably cause the one who filled it made the mistake
            // Set<?> set = (Set<?>) defaultValue;
            // if (!set.isEmpty()) {
            // for (Object object : set) {
            // if (!(object instanceof String))
            // throw new IllegalArgumentException(
            // "The given set does not contain strings only");
            // }
            // }
            return (T) getPrefs(ctx).getStringSet(key,
                (Set<String>) defaultValue);
        } else throw new IllegalArgumentException(defaultValue
            + " cannot be persisted in SharedPreferences");
    }
}

My questions are:

  1. Do I need to retrieve the preferences in a synchronized block (I understand that put/get operations are already synchronized)?
  2. When I get a null as a default in the retrieve method, I am not sure I adhere to the generics' contract - there is the possibility of a ClassCastException - but so is there in the original framework. Could I get completely rid of the CLASSES casts and still be as typesafe as Android? See my comments in code.
  3. generics' contract part II - if I get a Set as a parameter I believe I do not have to check if it is indeed a Set<String> - am I right?
  4. The order I check (via instanceof and isAssignableFrom) what kind of number I got is : Integer, Long, Float. Is my order correct? - think of a call like persist(ctx, key, 349857094387593475). Would completely dropping Integer checks do the trick (and be acceptable)?
  5. Do I handle the HONEYCOMB addition of Set<String> right (by throwing an exception if one calls my methods in pre HONEYCOMB build passing a Set in)?

Follow my TODOs in the code for the particular points I make.

Here is my current solution for anyone interested.

Part of this question is answered here.

\$\endgroup\$
  • \$\begingroup\$ Your self-answers were deleted because they contained code dumps, not actual reviews. This may be tolerated on SO, but it's not considered an acceptable answer on CR. \$\endgroup\$ – Jamal Feb 1 '15 at 18:15
  • \$\begingroup\$ @Jamal: Alright - at least though I did reedit the question to clarify the code is my solution as well as to re add the SO link - this SO link is to a question my actual questions here are answered - this was the "review" part of my answer (for whoever would actually click it ;) \$\endgroup\$ – Mr_and_Mrs_D Feb 1 '15 at 23:51
  • \$\begingroup\$ Fair enough. I've re-made them as hyperlinks instead of full URLs. \$\endgroup\$ – Jamal Feb 1 '15 at 23:57
4
\$\begingroup\$

I'm not too familiar with Android but I've found an issue with the code: both persist and retrieve methods contains the similar if-else chain. You should replace them with polymorphism:

To answer some of your questions:

  • (4) The order is not important since there isn't any class (in Java, at least) which instanceof more than one of Long, Integer and Float.

  • (5) If it's invalid input data, I'd throw an exception with a detailed error message to the caller.

\$\endgroup\$
  • \$\begingroup\$ Thanks ! This is not the final - I just left the copy paste for easy ref (they are not exactly same either) - (5) is an android thing - as for 4 if I get retrieve(ctx, key, 349857094387593475) will (value instanceof Integer) == true ? - that king of thing \$\endgroup\$ – Mr_and_Mrs_D Jul 20 '13 at 18:00
1
\$\begingroup\$

This code is will only work properly if you always use that same Context. getPrefs() caches the SharedPreferences instance for ContextA. But if it is called again with ContextB the value returned was the value retrieved when ContextA was used.


Using the synchronized modifier on a method synchronizes the method on the class instance (or the class object for static methods). Any code can also synchronize on these instances, potentially causing deadlocks in code you don't control. You should always synchronize on a private instance that only you control.


The huge blocks of comments tightly packed in with the code makes everything hard to read. If you are trying to ask for questions about the code, do not put that inside the code. The code you post should be how it would be in the code base.


Don't put all of you if blocks on one line. It makes it harder to read and can cause problems when you need to add in a second line.

else if (defaultValue instanceof Float) return (T) (Float) getPrefs(ctx)
            .getFloat(key, (Float) defaultValue);

Here the code is trying to be put on one line, but has to be wrapped. The defeats the whole purpose of trying to one-line the code in the first place. It is best to always put the block on the next line and to always include the curly brackets.

\$\endgroup\$
  • \$\begingroup\$ Yes thanks - half agree half disagree - point 1 though is important and IIRC it isn't so - the PreferenceManager.getDefaultSharedPreferences(ctx); should return an Application wide singleton. Please verify this - or I will do when I get round to it - do not have android setup anymore - but nice (re: code style including comments etc I have my reasons) - point 2 I was aware (and it is a internal class under my control) but yes - this is a +1 \$\endgroup\$ – Mr_and_Mrs_D Feb 1 '15 at 23:44
  • \$\begingroup\$ While a Activity should always have the same Context, there is nothing in this code to indicate to someone using it, that it should not be used with multiple Contexts. I don't know enough about Android to say how you might get a different context withing the same application. But the issue would not be a problem at all if you made the class non-static and passed a Context in as an argument to the constructor. The SharedPreferences instance would still be cached and it would be impossible to use the code incorrectly. \$\endgroup\$ – unholysampler Feb 2 '15 at 4:49

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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