Properly structuring SQLite classes/operations in Android

Question

I am new to Android development and I want to ensure I'm learning decent practices for doing things. Right now this is my database class, which currently allows me to make a new instance of the singleton, as well as create a profiles table, as well as add/retrieve from the profiles table.

This is my code so far:

public class DatabaseHelper extends SQLiteOpenHelper {
 private static volatile SQLiteDatabase mDatabase;
 private static DatabaseHelper mInstance = null;
 private static Context mContext;
 private static final String DB_NAME = "database.db";
 private static final int DB_VERSION = 1; 
 public static final String PROFILES_TABLE = "PROFILES";
 public static final String PROFILES_COLUMN_ID = "_ID";
 public static final String PROFILES_COLUMN_NAME = "NAME";
 private static final String DB_CREATE_PROFILES_TABLE = 
 "CREATE TABLE " + PROFILES_TABLE + " ("
 + PROFILES_COLUMN_ID + " INTEGER PRIMARY KEY AUTOINCREMENT, "
 + PROFILES_COLUMN_NAME + " TEXT UNIQUE NOT NULL)";
 public static synchronized DatabaseHelper getInstance(Context context) {
 if (mInstance == null) {
 mInstance = new DatabaseHelper(context.getApplicationContext());
 try {
 mInstance.open();
 }
 catch (SQLException e) {
 e.printStackTrace();
 }
 }
 return mInstance;
 }
 private DatabaseHelper(Context context) {
 super(context, DB_NAME, null, DB_VERSION);
 mContext = context;
 }
 @Override
 public void onCreate(SQLiteDatabase db) {
 db.execSQL(DB_CREATE_PROFILES_TABLE);
 }
 @Override
 public void onUpgrade(SQLiteDatabase db, int oldVersion, int newVersion) { 
 }
 @Override
 public void onConfigure(SQLiteDatabase db){
 super.onConfigure(db);
 db.setForeignKeyConstraintsEnabled(true);
 }
 public synchronized void open() throws SQLException {
 mDatabase = getWritableDatabase();
 }
 public synchronized void close() {
 mDatabase.close();
 }
 public synchronized long addNewProfile(String name) {
 ContentValues values = new ContentValues();
 values.put(DatabaseHelper.PROFILES_COLUMN_NAME, name);
 return mDatabase.insertWithOnConflict(DatabaseHelper.PROFILES_TABLE, null, values, SQLiteDatabase.CONFLICT_IGNORE);
 }
 public synchronized Profile getProfileById(long profileId) {
 Cursor cursor = mDatabase.query(
 DatabaseHelper.PROFILES_TABLE, // table
 new String[]{DatabaseHelper.PROFILES_COLUMN_ID, DatabaseHelper.PROFILES_COLUMN_NAME}, // column names
 DatabaseHelper.PROFILES_COLUMN_ID + " = ?", // where clause
 new String[]{profileId + ""}, // where params
 null, // groupby
 null, // having
 null); // orderby
 cursor.moveToFirst();
 Profile profile = null;
 if (!cursor.isAfterLast()) {
 String profileName = getStringFromColumnName(cursor, DatabaseHelper.PROFILES_COLUMN_NAME);
 profile = new Profile(profileId, profileName);
 cursor.moveToNext();
 }
 cursor.close();
 return profile;
 }
 public synchronized List<Profile> getAllProfiles() {
 List<Profile> profiles = new ArrayList<>();
 Cursor cursor = mDatabase.query(
 DatabaseHelper.PROFILES_TABLE, // table
 new String[]{DatabaseHelper.PROFILES_COLUMN_ID, DatabaseHelper.PROFILES_COLUMN_NAME}, // column names
 null, // where clause
 null, // where params
 null, // groupby
 null, // having
 DatabaseHelper.PROFILES_COLUMN_NAME); // orderby
 cursor.moveToFirst();
 while (!cursor.isAfterLast()) {
 long profileId = getLongFromColumnName(cursor, DatabaseHelper.PROFILES_COLUMN_ID);
 String profileName = getStringFromColumnName(cursor, DatabaseHelper.PROFILES_COLUMN_NAME);
 profiles.add(new Profile(profileId, profileName));
 cursor.moveToNext();
 }
 cursor.close();
 return profiles;
 }
 private synchronized long getLongFromColumnName(Cursor cursor, String columnName) {
 int columnIndex = cursor.getColumnIndex(columnName);
 return cursor.getLong(columnIndex);
 }
 private synchronized String getStringFromColumnName(Cursor cursor, String columnName) {
 int columnIndex = cursor.getColumnIndex(columnName);
 return cursor.getString(columnIndex);
 }
}

For reference (this may or may not be necessary, but I am posting it just in case), my Profile class, which is something I use in several other places in the app:

public class Profile {
 private long mId;
 private String mName;
 public Profile(long id, String name) {
 mId = id;
 mName = name;
 }
 public long getId() {
 return mId;
 }
 public void setId(long id) {
 mId = id;
 }
 public String getName() {
 return mName;
 }
 public void setName(String name) {
 mName = name;
 }
}

My questions:

1. Is it proper to be storing the field names of the table in the database class like this, or should I be moving it to its own separate class (for example a ProfileSql class of some kind that holds all the names).

2. Should I be decoupling the query logic from this class somehow? How do I do this? What if I have several tables, queries, thread methods, etc? Do these all go in their own separate classes, too? If I add CRUD functions for several tables, this class could get very large very quickly.

3. Should I be somehow tying any of this stuff into my Profile class itself, which I use in several other places in my app? For instance should I be including the profile table SQL stuff (the create table string and the table/column names) in the Profile class, or is this meddling things together that shouldn't be?

As you can see I am mostly asking where stuff should go. Right now I am just kind of lumping it all together into one database class.

I am hoping that this example is short enough to where someone can show me the proper way to restructure all of this so I can take those skills and apply them going forward.

Answer 1

Is it proper to be storing the field names of the table in the database class like this, or should I be moving it to its own separate class (for example a ProfileSql class of some kind that holds all the names).

What you have is fine.

Should I be decoupling the query logic from this class somehow? How do I do this? What if I have several tables, queries, thread methods, etc? Do these all go in their own separate classes, too? If I add CRUD functions for several tables, this class could get very large very quickly.

You could, but what you have is fine, too. People generally either put all their DB stuff together and leave their POJOs "clean", or they do it more ORM style (by putting db methods on the model objects - like your Profile class). It's really a matter of preference. For something as small as this, I'd probably do it how you have it.

Should I be somehow tying any of this stuff into my Profile class itself, which I use in several other places in my app? For instance should I be including the profile table SQL stuff (the create table string and the table/column names) in the Profile class, or is this meddling things together that shouldn't be?

See the above.

You didn't ask, but I do wonder about your thread-safety strategy - do you feel confident in it generally?

For the singleton itself, another user here pointed out this strategy (double-checked locking), and I quite like this one (initialization-on-demand holder idiom).

• user6419910: People generally either put all their DB stuff together and leave their POJOs "clean", or they do it more ORM style (by putting db methods on the model objects - like your Profile class). what do you mean by this? As in putting anything remotely Profile related (database table/column names, member variables, access methods, querying methods / thread methods, etc) in the Profile class?
• user6419910: As for thread safety I am not experienced with it yet, so probably not
• mdd-sbo: i'd say people either put all the DB stuff together, or give their models classes db-related methods, e.g., Profile.save(), which would save the current state of the Profile instance to your db, using whatever mechanisms were in place. Honestly I think either is fine. As far as thread-safety, I'd say you're probably fine, and maybe even doing more than is necessary - if you don't expect to be doing things from a multiple threads, then you might be able to scale back some - that said, you can optimize later
• user6419910: It's an Android app, so I do need to make it multithreadable because otherwise everything runs on the "main UI thread" which may make the app appear unresponsive. Ideally database operations are done on separate threads so the UI can proceed without a hiccup.
• user6419910: So in something like Profile.save() it would call a method that, for example, accepts a Context, gets the instance of the database / my Helper object, and then calls a querying/Cursor method that is implemented inside the Profile class? Am I understanding you right?

Answer 2

1. Yes, I'd move the SQL information in ProfileColumns that extends BaseColumns
2. Yes, the responsibility of the DbOpenHelper is to open and upgrade the database. It's usually not this class that fetches objects from the database. The addProfile() method should probably be in the Profile object itself, or in a ProfileDbHelper if you want to leave your Profile as a DTO.
3. The name of the Profile TABLE can remain in your DbOpenHepler, or be moved in ProfileDbHelper.

Also:

• having a singleton DbOpenHelper is good, but your implementation of the singleton pattern is not thread-safe. See Singleton pattern on Wikipedia for a correct implementation
• catching and swallowing exceptions is a terribly bad practice
• similarly, getStringFromColumnName would be better in a CursorWrapper
• I think you can simplify your Profile DTO as:

Simpler Profile DTO:

 public class Profile {
 public long mId;
 public String mName;
 }

And more minor comments:

• reuse BaseColumns._ID for the "id" column name rather than introducing your own "_ID" constant
• specify the capacity of new ContentValues()
• static fields are prefixed by s, not m

And some personal preference comments:

• I would better see the getApplicationContext() in the constructor rather than in getInstance()

• user6419910: How is it not threadsafe? I used an implementation I've seen everywhere (including by CommonsWare) that is implemented this way. Also the error is not swallowed -- I print the stacktrace