I have an Android app with three different Activity
classes. Each of the Activities uses and changes config and user-customization data stored in a wrapped SharedPreference
s. The code could stand some refactoring, so I'm thinking that rather than have each Activity
instantiate the SharedPreference
s by calling PreferenceManager.getDefault...
in their constructors, I can make a 'base' abstract Activity to serve as a parent class and then have each of the Activities subclass the abstract Activity
and wrap the SharedPreference
s in an inner class of the abstract Activity
, like so:
public abstract class AbstractBaseActivity extends AppCompatActivity
{
private static AppPrefs instance;
private interface PrefKeys
{
String KEY_BOOL_SETUP = "KEY_BOOL_SETUP";
String KEY_STR_USER_EMAIL = "KEY_STR_USER_EMAIL";
String KEY_LONG_LAST_EMAIL_SENT = "KEY_LONG_LAST_USE";
//...remaining keys omitted...//
}
public static class AppPrefs implements PrefKeys
{
private SharedPreferences prefs;
private String no_email;
private AppPrefs(AbstractBaseActivity activity)
{
prefs = PreferenceManager.getDefaultSharedPreferences(activity);
no_email = activity.getString(R.string.no_email);
}
public static AppPrefs getInstance(AbstractBaseActivity activity)
{
if(instance == null)
{
instance = new AppPrefs(activity);
}
return instance;
}
public String getUserEmail()
{
return prefs.getString(AppPrefs.KEY_STR_USER_EMAIL, no_email);
}
public boolean setUserEmail(String email)
{
return prefs.edit().putString(AppPrefs.KEY_STR_USER_EMAIL, email).commit();
}
//... getter/setter methods to modify the SharedPreferences ...//
}
}
I like this approach because it simplifies getting the SharedPreference
s. I just call AppPrefs.getInstance(this)
from within the Activities or AppPrefs.getInstance(getActivity())
from within a Fragment
. Currently, my SharedPreferences
are wrapped in a custom Object
-- AppPrefs
-- which uses obviously named getter
s and setter
s to interact with my SharedPreference
data without actually having to muck about with key-value pairs and remembering to call commit()
when changing data, but it is a stand-alone class. It seems to make sense to make the AppPrefs
class an inner class of the AbstractBaseActivity
class, but I'd welcome any criticisms of the approach.
I'm also uncertain about how I'm dealing with getting the AppPrefs
object -- is there a better way to implement instantiation?
What are the downsides I'm not seeing?
-
\$\begingroup\$ Since the question involves refactoring a top-level singleton into an inner class, should I just delete the question? \$\endgroup\$user5292387– user52923872015年09月06日 19:43:41 +00:00Commented Sep 6, 2015 at 19:43
-
1\$\begingroup\$ It's not a huge problem. As long as questions do a good job of displaying and explaining the code in question, the rest is either fine, or it'll get edited. \$\endgroup\$Kaz– Kaz2015年09月06日 19:46:39 +00:00Commented Sep 6, 2015 at 19:46
-
2\$\begingroup\$ @user5292387 No. Your title should just describe what the code does, in brief. \$\endgroup\$Ethan Bierlein– Ethan Bierlein2015年09月06日 19:47:10 +00:00Commented Sep 6, 2015 at 19:47
-
\$\begingroup\$ @EthanBierlein, that's what has me confused, my question is not about what my code does, but rather on the choice I'm making about how to refactor it. So I'm not exactly sure how to write a title that satisfies some requirement that can't be met. \$\endgroup\$user5292387– user52923872015年09月06日 19:59:40 +00:00Commented Sep 6, 2015 at 19:59
-
\$\begingroup\$ @user5292387 The question doesn't necessarily have to describe what your code does, but generally, the title of your question should describe what the code does. \$\endgroup\$Ethan Bierlein– Ethan Bierlein2015年09月06日 20:03:01 +00:00Commented Sep 6, 2015 at 20:03
1 Answer 1
Public class, private Interface?
I'm pretty confused by this setup.
The meaning is always trying working towards the most highest object( this case interface) possible.
I'm thinking that you created this setup so having the keys as constants.
But there is nothing wrong to have those keys as private static final String
in your class self.
Java singleton.
I can't repeat it enough but if you want a singleton, please think enum
in stead of private constructor.
Your singleton isn't thread safe or Serializable
in this case, while if you use enum
it is, and it's even easier to created it.
Abstract class root of all evil?
While I do use abstract classes, I'm always thinking also about putting the abstract methods in an interface and use that interface in the constructor.
This has multiple advantages of the abstract class.
First of all, you can create the base class final so no unexpected behavior could be implemented later on.
Then the interface you can swap at runtime (don't forget a check on null
) and your class behave different but still in the correct way.
As I don't see the whole picture here I can't say it's good or not in this case.
But still, think about it, see the advantages/disadvantages and make your conclusion if it's good in this case or not.