-1

Hi I have the following code and as I am stepping through it in the debugger I notice that the constructor does not get invoked and hence mContext variable initiated within it remains null.

As I am stepping through the debugger the getInstance() function will call the constructor RaceTimeDataContract(Context context), however if I try to step into the constructor it does not and instead the debugger steps to the line where TABLE_NAME is being initialized. The problem is since mContext remains null, then exception is being thrown.

Anyone know what may be causing this behavior? Code is below:

public class RaceTimeDataContract implements BaseColumns{
   private static RaceTimeDataContract sInstance;
   private static Context mContext;

   private RaceTimeDataContract(Context context) {
      this.mContext = context;   // This is not getting called
   }
   private RaceTimeDataContract(){}

   public static RaceTimeDataContract getInstance(Context context) {
      if (sInstance == null) {
         sInstance = new RaceTimeDataContract(context.getApplicationContext());
      }
      return sInstance;
   }
   // mContext remains null and forces a exception
   private final String TABLE_NAME = mContext.getResources().getString(R.string.table_name);

Appreciate any feedback!

tomerpacific
  • 4,704
  • 13
  • 34
  • 52
Steve Malek
  • 69
  • 5
  • First of all, it is a bad practice to save a context reference in your singleton. Second, you have two constructors for your singleton. Why do you need both? Also, did you make sure that the context you are passing is not null? – tomerpacific Feb 26 '22 at 19:41
  • @tomerpacific I am passing context to the class because my goal here is to try to gain access to the resources string so that I don't hardcode values. Regarding the 2 constructors I had changed the initial code that I used based on Google's Android Sqlite tutorial and forgot to remove the constructor based on that code. The context being passed is not null. What is your recommendation? Btw, thanks for your feedback! – Steve Malek Feb 26 '22 at 20:58

1 Answers1

-1

When creating new instances, inline field initialisation is performed before the constructor is called. One simple fix in your case is to move the field initialisation into the constructor after the context is set

e.g. (I've reorganised the code a little, and dropped the this on mContext since it is a static member)

public class RaceTimeDataContract implements BaseColumns {
    private static RaceTimeDataContract sInstance;
    private static Context mContext;

    public static RaceTimeDataContract getInstance(Context context) {
        if (sInstance == null) {
            sInstance = new RaceTimeDataContract(context.getApplicationContext());
        }
        return sInstance;
    }

    private final String TABLE_NAME;

    private RaceTimeDataContract(Context context) {
        mContext = context;
        this.TABLE_NAME = mContext.getResources().getString(R.string.table_name);
    }

     // The below is invalid as it doesn't set TABLE_NAME and should probably be removed
    // private RaceTimeDataContract(){}
}

The above isn't very idiomatic and would be better re-arranged to support a singleton better. There are many ways to do this but some general things which would be a benefit are

  • Storing the context on the instance object, and not statically
  • Preventing race conditions on getInstance
  • Removing the default constructor

Assuming you don't retrieve the instance often this can easily be accomplished by synchronising on the getInstance call. e.g.

public class RaceTimeDataContract implements BaseColumns {
    private static RaceTimeDataContract sInstance;

    public static synchronized RaceTimeDataContract getInstance(Context context) {
        if (sInstance == null) {
            sInstance = new RaceTimeDataContract(context.getApplicationContext());
        }
        return sInstance;
    }

    private final Context mContext;
    private final String tableName;

    private RaceTimeDataContract(Context context) {
        this.mContext = context;
        tableName = mContext.getResources().getString(R.string.table_name);
    }
}

As a last note it is atypical of a singleton to take a parameter in the getInstance method but always return the same object - you can imagine in this case that if getInstance is called a second time with a different context then we get the original instance which may have an incorrect table name.

If you do need a singleton per context you can store each instance per context in something like a Map<Context, RaceTimeDataContract>, probably using a ConcurrentHashMap, and it would be better to think of it as some kind of reference cache or factory rather than a singletone.

If possible, it is better to just have a zero parameter getInstance method which can retrieve the singleton context statically the first time it is required. This can open your class up to more typical/simpler singleton patterns like enum singletons and static singleton holders.

Barnesly
  • 83
  • 1
  • 6