current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 3 down vote favorite
2

I am trying to optimize inserts from a returned JSON which contains data from multiple tables on the remote server. My code is working, but I just want to figure out a way for it to work better. I need the structure to be very dynamic (i.e. fetching the table names from JSON then looping through to complete the inserts into SQLite). This is so if a table is added in the future, I will not have to change any code.

private boolean prepReturnedData(String newString) throws JSONException {
     long errCheck = 0; // tracks valid db inserts.

     JSONArray columns = new JSONArray();

     JSONObject jsonObj = new JSONObject(newString);
     ArrayList<String> tables = new ArrayList<String>();


     Iterator<?> tableKeys = jsonObj.keys();

     while (tableKeys.hasNext()) {
         tables.add(tableKeys.next().toString());
     }
     for (String table : tables) { // Loop through table names.
         columns = jsonObj.getJSONArray(table);
         for (int index = 0; index < columns.length(); index++) { // Loop through Array of Entries

             ContentValues cv = new ContentValues(); // Figure out where to put this.

             Iterator<?> iter = columns.getJSONObject(index).keys(); // Creates an Iterator containing Column names.
             JSONObject newObj = columns.getJSONObject(index); // Creates a new JSONObject for retrieving column values.

             while (iter.hasNext()) { // Iterates to get Column names.
                 String tColumn = iter.next().toString(); // Stores column name, for retrieval of column value.

                 cv.put(tColumn, newObj.get(tColumn).toString());

             } // end column iterator loop.

             String status = null;
             if (table == "Seizures") {
                 status = "timestamp";
             } else {
                 status = "name";
             }
             try {
                 SQLiteDatabase db = this.getWritableDatabase();                
                 errCheck = db.replace(table, status, cv);

                 if (db.isOpen()); {db.close(); };
             } catch (Exception e) {
                 // TODO Auto-generated catch block
                 e.printStackTrace();
             }
         } // end entry loop
     } // end table loop
     if (errCheck >= 0) {
         return true;
     } else {
         return false;
     }
}

This function is within a DatabaseHelper class.

share|improve this question

migrated from stackoverflow.com May 26 '12 at 0:32

This question came from our site for professional and enthusiast programmers.
    
Every real database has a support for something that is called bulk insert. Look for it, check out this blog blog.quibb.org/2010/08/fast-bulk-inserts-into-sqlite –  Leonid May 25 '12 at 16:53

2 Answers 2

up vote 4 down vote accepted

I suggest some optimizations:

  • Do not initialize columns with empty JSONArray - it will always be overwritten.

  • Do not iterate twice and do not create overhead ArrayList object you can iterate json object once.

  • Due to Android specific garbage collection it's better to reuse object than create a new one in each iteration (see where is declared ContentValues cv = new ContentValues(); and cleared then later).

  • Open database once not in loop -> be sure to close it (finally block).

  • Your error reporting is leaky - the next db.replace would clear off the former errCheck value - I added some simple handling but I did not know the requirements in that area.

    private boolean prepReturnedData(String newString) throws JSONException {
    JSONObject jsonObj = new JSONObject(newString);

    if(jsonObj.length() > 0) {
        Iterator<?> tableKeys = jsonObj.keys();
        SQLiteDatabase db = null;

        try {
            db = getWritableDatabase();
            db.beginTransaction();

            ContentValues cv = new ContentValues();

            String table;
            JSONArray columns;

            String status;
            int columnsLength;
            JSONObject columnJson;
            Iterator<?> columnKeys;

            String tColumn;

            while (tableKeys.hasNext()) {
                table = tableKeys.next().toString();
                columns = jsonObj.getJSONArray(table);
                status = table == "Seizures" ? "timestamp" : "name";
                columnsLength = columns.length();

                for (int i = 0; i < columnsLength; i++) { // Loop through Array of Entries
                    cv.clear();

                    columnJson = columns.getJSONObject(i); // Creates a new JSONObject for retrieving column values.
                    columnKeys = columnJson.keys(); // Creates an Iterator containing Column names.

                    while (columnKeys.hasNext()) {
                        tColumn = columnKeys.next().toString();
                        cv.put(tColumn, columnJson.optString(tColumn, ""));
                    }


                    if(db.replace(table, status, cv) == -1L) {
                        throw new IllegalStateException(
                                "Insert failed for table: " + table + 
                                ", contentValues: " + cv.toString());
                    }
                }
            }

            db.setTransactionSuccessful();
        }
        catch(Throwable e) {

            return false;
        }
        finally {
            if (db != null && db.isOpen()) {
                db.endTransaction();
                db.close(); 
            };
        }
    }

    return true;
}
share|improve this answer
    
Thank you this seems like a very sensible, and less error prone approach. –  cmac147 May 25 '12 at 17:37
up vote 1 down vote

When in doubt, run SQL statements, of either form:

  • insert into table (col1, col2, ...) values ('val11', 'val12', ...), ('val21', 'val22', ...)
  • insert into table (col1, col2, ...) values (@val1, @val2)

Theoretically a proper DBMS would cache the plan for #2 and you could just loop through your values feeding the parameters one by one and executing each in row. Sqlite has different goals though, so the first version could be better (faster?), try em and see.

share|improve this answer
    
This sqlite under android I am working on. As far as I know you cannot simply run sql statements in this manner. –  cmac147 May 25 '12 at 15:51
    
Are you sure? I see an execSQL method in the documentation. –  Blindy May 25 '12 at 15:52
    
It is too early in the morning, you are correct about execSQL, I even use it in multiple places in my code. I am just wondering whether there would be any real change in efficiency by running a loop to assemble the large query then executing it, or using the prebuilt db.replace method in a loop. –  cmac147 May 25 '12 at 16:06
    
Wouldn't know, I have an iPhone (nor do I use the desktop version of Sqlite). Try it and see, it's really not that much code to write. –  Blindy May 25 '12 at 16:19
    
@Blindy: execSQL call allows only single sql command! –  Tomasz Gawel May 25 '12 at 16:51

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.