current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
up vote 5 down vote favorite
1

I'd like some opinions about my approach at using SQLite databases in android. Everything seems to work very well, but there may be something wrong that I hadn't noticed.

I don't like working with create/alter table statements in code, so I prefer creating my database with some SQLite client out there and putting it in assets folder. I use some logic to decide when the database must be replaced and which records need to be kept, then use this code in a class called DbManager:

private static SQLiteDatabase _db = null;

public static Object lock = new Object();

private static SQLiteDatabase getDB() {
    if (_db == null) {
        _db = SQLiteDatabase.openDatabase(DB_PATH + DB_NAME, null, SQLiteDatabase.OPEN_READWRITE);
    }

    return _db;
}


//this is the approach used for insert operations, but is the same for update, delete etc
public static boolean insert(String tableName, ContentValues values) {
    try {
        return getDB().insert(tableName, null, values) >= 0;
    } catch (Exception e) {
        LogHelper.WriteLogError("error in DB_manager.insert function", e);
        return false;
    }
}

public static synchronized void beginTransaction(String who) {
    LogHelper.WriteLogDebug("starting transaction by " + (who == null ? "null" : who));
    try {
        //in this way if a transaction is still executing it waits untill the previus ends.
        //obviously every time i call DbManager.beginTransaction the call at DbManager.endTransaction is in the finally statment
            if (_db.inTransaction()) {
                LogHelper.WriteLogInfo("DB IS IN TRANSACTION");
                synchronized (lock) {
                    lock.wait();
                }
            }
            getDB().beginTransactionWithListener(new SQLiteTransactionListener() {
                @Override
                public void onRollback() {
                    synchronized (lock) {
                         LogHelper.WriteLogInfo("onRollback listener invoked");
                         lock.notifyAll();
                    }
                }

                @Override
                public void onCommit() {
                    synchronized (lock) {
                        LogHelper.WriteLogInfo("onCommit listener invoked");
                        lock.notifyAll();
                    }
                }

                @Override
                    public void onBegin() {
                        // TODO Auto-generated method stub
                    }
                });
            } catch (Exception e) {
                LogHelper.WriteLogError("error calling begin transaction by " + (who == null ? "null" : who), e);
            }
        }

        public static void endTransaction(boolean commitChanges, String who) {
            LogHelper.WriteLogDebug("ending transaction by " + (who == null ? "null" : who));
            try {
                if (commitChanges)
                    getDB().setTransactionSuccessful();
                getDB().endTransaction();
            } catch (Exception e) {
                LogHelper.WriteLogError("error calling end transaction by " + (who == null ? "null" : who), e);
            }
        }
share|improve this question
up vote 5 down vote

The code looks fine to me but it is a bit odd because the official docs recommend creating a SQLiteOpenHelper subclass to handle database interaction. And if you use one SQLiteOpenHelper instance across your application, it will be thread-safe so there's no need to add your own synchronization logic:

public class DatabaseHelper extends SQLiteOpenHelper {

  private static final String DATABASE_NAME = "location_scout.db";
  private static final int SCHEMA_VERSION = 1;

  // Singleton pattern
  private static DatabaseHelper instance;
  public synchronized static DatabaseHelper getInstance(Context context) {
      if(instance == null) {
          instance = new DatabaseHelper(context.getApplicationContext());
      }
      return instance;
  }

  private DatabaseHelper(Context context) {
      super(context, DATABASE_NAME, null, SCHEMA_VERSION);
  }

}
share|improve this answer
    
Could you add a link to the docs? Also, is it really necessary to use it as singleton? – Simon Forsberg♦ Nov 28 '13 at 0:48
1  
@SimonAndréForsberg: It's not required but it's the easiest way to make it thread-safe and not run into memory leak issues. More info: androiddesignpatterns.com/2012/05/… – seand Nov 28 '13 at 2:01
    
Damn yes! I use SQLight a lot on Android. But somehow I missed this... Thanks for this hint. – Marcel Blanck Nov 28 '13 at 20:48

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.