1
\$\begingroup\$

I wrote this simple key-value interface on top of Sqlite3 to use in my Android app. I am using this to persist Java objects as strings converted using Gson.

The public API has 3 methods.

  1. get(Context context, String key, String defaultValue)
  2. set(Context context, String key, String value, Integer persist)
  3. clearCacheByLimit(Context context, long limit)

I attempted to make the API thread safe (which is also to be reviewed) and I am planning to make this module open source soon. Hence, a code review before I do it. We have been using this code base in production now for 4+ month (as on 20th September 2015) in our all sports (more than 20) news app Sportskeeda

public class KeyValueDB extends SQLiteOpenHelper {
    private static final String TAG = "KeyValueDB";
    private static KeyValueDB sInstance;

    private static final String DATABASE_NAME = "app";
    private static final String DATABASE_TABLE = "cache";
    private static final int DATABASE_VERSION = 1;

    private static final String KEY = "KEY";
    private static final String VALUE = "VALUE";
    private static final String PERSIST = "PERSIST";
    private static final String KEY_CREATED_AT = "KEY_CREATED_AT";


    private static final String CREATE_TABLE = "CREATE TABLE "
            + DATABASE_TABLE + "(" + KEY + " TEXT PRIMARY KEY," + VALUE
            + " TEXT," + PERSIST + " INTEGER," + KEY_CREATED_AT
            + " DATETIME" + ")";

    private static synchronized KeyValueDB getInstance(Context context) {
        // Use the application context, which will ensure that you
        // don't accidentally leak an Activity's context.
        // See this article for more information: http://bit.ly/6LRzfx
        if (sInstance == null) {
            sInstance = new KeyValueDB(context.getApplicationContext());
        }
        return sInstance;
    }

    /**
     * Constructor should be private to prevent direct instantiation.
     * make call to static method "getInstance()" instead.
     *
     * @param context Any context object.
     */
    private KeyValueDB(Context context) {
        super(context, DATABASE_NAME, null, DATABASE_VERSION);
    }

    @Override
    public void onCreate(SQLiteDatabase db) {
        Log.v(TAG, "onCreate");
        db.execSQL(CREATE_TABLE);
    }

    @Override
    public void onUpgrade(SQLiteDatabase db, int oldVersion, int newVersion) {
        Log.v(TAG, "onUpgrade");
        db.execSQL("DROP TABLE IF EXISTS " + DATABASE_TABLE);
        onCreate(db);
    }

    /**
     * Setter method. Sets a (key, value) pair in sqlite3 db.
     *
     * @param context Any context object.
     * @param key     The URL or some other unique id for data can be used
     * @param value   String data to be saved
     * @param persist Whether to delete this (key, value, time, persist) tuple, when cleaning cache in
     *                clearCacheByLimit() method. 1 Means persist, 0 Means remove.
     * @return rowid of the insertion row
     */
    public static synchronized long set(Context context, String key, String value, Integer persist) {
        key = DatabaseUtils.sqlEscapeString(key);
        Log.v(TAG, "setting cache: " + key);
        KeyValueDB dbHelper = getInstance(context);
        SQLiteDatabase db = dbHelper.getWritableDatabase();
        long row = 0;
        if (db != null) {
            ContentValues values = new ContentValues();
            values.put(KEY, key);
            values.put(VALUE, value);
            values.put(PERSIST, persist);
            values.put(KEY_CREATED_AT, "time('now')");
            row = db.replace(DATABASE_TABLE, null, values);
            Log.v(TAG, "save cache size: " + String.valueOf(value.length()));
            db.close();
        }
        return row;
    }

    /**
     * @param context      Any context object.
     * @param key          The URL or some other unique id for data can be used
     * @param defaultValue value to be returned in case something goes wrong or no data is found
     * @return value stored in DB if present, defaultValue otherwise.
     */
    public static synchronized String get(Context context, String key, String defaultValue) {
        key = DatabaseUtils.sqlEscapeString(key);
        Log.v(TAG, "getting cache: " + key);
        KeyValueDB dbHelper = getInstance(context);
        SQLiteDatabase db = dbHelper.getReadableDatabase();
        String value = defaultValue;
        if (db != null) {
            Cursor c = db.query(DATABASE_TABLE, new String[]{VALUE}, KEY + "=?", new String[]{key}, null, null, null);
            if (c != null) {
                if (c.moveToNext()) {
                    value = c.getString(c.getColumnIndex(VALUE));
                }
                Log.v(TAG, "get cache size:" + String.valueOf(value.length()));
                c.close();
            }
            db.close();
        }
        return value;
    }

    /**
     * Clear the cache like a FIFO queue defined by the limit parameter.
     * Each function call made to this will remove count(*)-limit first rows from the DB
     * Only the data with (Persist, 0) will be removed
     *
     * @param context Any context object.
     * @param limit   amount of data to be retained in FIFO, rest would be removed like a queue
     * @return number of rows affected on success
     */
    public static synchronized long clearCacheByLimit(Context context, long limit) {
        KeyValueDB dbHelper = getInstance(context);
        SQLiteDatabase db = dbHelper.getWritableDatabase();
        long numRows = 0;
        if (db != null) {
            Cursor c = db.query(DATABASE_TABLE, null, null, null, null, null, null);
            if (c != null) {
                long count = c.getCount();
                Log.v(TAG, "cached rows" + String.valueOf(count));
                if (count > limit) {
                    String ALTER_TBL = "DELETE FROM " + DATABASE_TABLE +
                            " WHERE " + KEY + " IN (SELECT " + KEY + " FROM " + DATABASE_TABLE + " WHERE " + PERSIST + " = 0" + " ORDER BY " + KEY_CREATED_AT + " ASC LIMIT " + String.valueOf(count - limit) + ");";
                    db.execSQL(ALTER_TBL);
                }
                c = db.query(DATABASE_TABLE, null, null, null, null, null, null);
                numRows = count - c.getCount();
                c.close();
            }
            db.close();
        }
        return numRows;
    }
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Two things that bother me about this question: Ad 1) You mention 3 "Crashes" found by Crashlytics. Please clarify whether these are actual hindrances in what your code seeks to accomplish. If your code is not ready for use without these 3 issues fixed, then I'd be inclined to consider the question off-topic. Ad 2) Be aware that answers may comment on any or all aspects of the code! You may not get what you want. \$\endgroup\$ – Vogel612 Sep 20 '15 at 19:45
  • 1
    \$\begingroup\$ Thanks for writing. Sure, I am aware of the licence and entitled to share the code base. The code is already in production and we have not faced any major issue. I saw the above mentioned crashes but could not find if they are avoidable. On the whole, the aim of asking for this code review is to make it better, I do not seek anything else. Moreover, I feel it is close to be open sourced. So, others may use it as well. \$\endgroup\$ – divyenduz Sep 20 '15 at 19:50
  • \$\begingroup\$ @Vogel612 As well, I can not think of a better use of code review other than making it better ? Please tell me more about the use case of this web site. \$\endgroup\$ – divyenduz Sep 20 '15 at 19:52
  • \$\begingroup\$ Moreover, tell me if removing a part of my question makes it a better fit for this web site \$\endgroup\$ – divyenduz Sep 20 '15 at 19:53
  • \$\begingroup\$ I'm not working with android. This means I do not know what to parse these Crashlytics things to. If they were something like findbugs warnings, then it may be simpler to remove mention of these for clarity. Users aroung here are a little trigger-happy when it comes to closing questions that ask us to fix code that's not doing what it's supposed to. But yes, Code Review seems to be the best fit for this question as of now. \$\endgroup\$ – Vogel612 Sep 20 '15 at 19:58
2
\$\begingroup\$

You can improve the performance of this piece of code don't using the synchronized for the getInstance method but just the first time when the sInstance is still null.

private volatile static KeyValueDB sInstance;

private static KeyValueDB getInstance(Context context) {
    // Use the application context, which will ensure that you
    // don't accidentally leak an Activity's context.
    // See this article for more information: http://bit.ly/6LRzfx
    if (sInstance == null) {
        synchronized (KeyValueDB.class) {
            if (sInstance == null)
                sInstance = new KeyValueDB(context.getApplicationContext());
        }
    }
    return sInstance;
}
\$\endgroup\$
  • \$\begingroup\$ Thanks for the answer Dave. I would still wait for a few more answers to pop up before accepting though. This project is open sourced now at github.com/divyenduz/KeyValueDB Feel free to send in a pull request if you feel like. \$\endgroup\$ – divyenduz Sep 28 '15 at 21:53

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.