Code for saving / loading a class object

Question

As I was advised, I broke my code into 2 classes. Could you take a look at the repository class and report what problems this code has

public class JsonSharedPreferences {
private static Gson gson = new Gson();
private Context context;
public JsonSharedPreferences(Context context) {
 this.context = context.getApplicationContext();
}
public <T> T loadObject(Class<T> classType, String key) {
 SharedPreferences settings = context.getSharedPreferences(key, Context.MODE_PRIVATE);
 String json = settings.getString(key, null);
 return gson.fromJson(json, classType);
}
 public <T> void saveObject(T objective, String key) {
 SharedPreferences settings = context.getSharedPreferences(key, Context.MODE_PRIVATE);
 String json = gson.toJson(objective);
 settings.edit().putString(key, json).apply();
 }
}

• Jude Niroshan: Objective class does not hold a single responsibility. It acts as a POJO and Service layer class. I would create 2 different classes and segregate the functionalities. Using static will tempt to cause runtime rabbit holes. Therefore, verify your static methods would work properly on concurrent scenarios.
• mtj: Sorry, needed to rollback, as changing code to incorporate answers (or in this case: helpful comments) is not done on this site.
• Malachi: @mtj we don't have the same rules about comments that we do for answers, please do not rollback for a comment's sake.

Answer 1

• Why saving reference to Context and then create SharedPreferences each time? There's duplicate code for creating SharedPreferences You could just use context in constructor to create member SharedPreferences to use in your methods. If you want to keep it as it is, at least extract duplicate code into separate method.
• I am missing null checks. I expect to get very obscure behaviour when passing null for keys or classes. Test for null and throw NullPointerException early, be fail-fast. For example checkNotNull or some NotNull annotation to help better fighting against null. Another way to fight is to use kotlin, which doesn't allow null on compiler level unless you make variable optional.

• I don't like that gson is private and static.

  • That means there is no way to configure gson. Would be better if this was member property and I would be able to pass different configuration of gson. Can add consructor overloads for that. Maybe this is overdoing it for your case, but I usually configure Gson quite a lot, have my own TypeAdapters and I wouldn't be able to use my Gson configuration with your class.
  • At first I was concerned about thread-safety Gson is thread-safe, so there's no argument against static in this sense. Always check for that when you make something static like this.

• Pretty sure that when saving preferences in android, you first put key and then value. Your saveObject has it the other way around, that is imho confusing. I'd switch parameters in loadObject too so that key is always first in those cases, but guess that's a bit subjective.

• Immutability makes code simpler to use and read. If something is final, you don't need to keep be alert if it changed anytime. context member variable can be final and all method parameters can be final too. After refactoring possible SharedPreferences and Gson instances can also be final - set in constructor.

Answer 2

Hellow K.H. thanks for the answer. I rewrote the code using Dagger 2 for dependency injection and following your advice. Could you take a look at this code and answer a couple of questions:

• How to correctly pass name to JsonSharedPreferences (Make a setter, make a module that will provide name).
• Is it correct to select this code as a separate component or more correctly add these modules to the application component(I use application component to provide context and application)

JsonSharedPreferences:

public class JsonSharedPreferences {
 private Gson gson;
 private SharedPreferences settings;
 public JsonSharedPreferences(@NonNull Context context, @NonNull String name, @NonNull Gson gson) {
 SharedPreferences settings = context.getSharedPreferences(name, Context.MODE_PRIVATE);
 this.settings = settings;
 this.gson = gson;
 }
 public <T> T loadObject(@NonNull String key, @NonNull Class<T> classType) {
 String json = settings.getString(key, null);
 return gson.fromJson(json, classType);
 }
 public <T> void saveObject(@NonNull String key, @NonNull T objective) {
 String json = gson.toJson(objective);
 settings.edit().putString(key, json).apply();
 }
}

JsonSharedPreferencesModule:

@Module
public class JsonSharedPreferencesModule {
 @Provides
 JsonSharedPreferences provideJsonSharedPreferences(Context context, String name, Gson gson) {
 return new JsonSharedPreferences(context, name,gson);
 }
}

GsonModule:

@Module
public class GsonModule {
 @Provides
 Gson provideGson() {
 return new Gson();
 }
}

• K.H.: Not sure if this is how site should be used. This is probably for another separate question. I will try to answer anyway. I think passing name in constructor is fine as you have it now. Sorry cannot give good answer about the other question, my knowledge is limited there. You can also make all parameters and member variables final. Immutability is always good :-) Actually I will add that to my original answer.