Android database access

For my simple Android application, I don't want to use an ORM. I'd like to have a db-communcation layer easy to user, to read and efficient.

This is my solution: every entity (ex: Person) as an helper that do the CRUD functions (ex: PersonHelper). The helper extends another class (EntityHelper), that contains the logic not related to the specific entity.

This is the code for the EntityHelper:

```
package com.dw.android.db;

import java.util.ArrayList;
import java.util.List;

import android.content.ContentValues;
import android.database.Cursor;
import android.database.sqlite.SQLiteDatabase;
import android.database.sqlite.SQLiteOpenHelper;
import android.database.sqlite.SQLiteQueryBuilder;

import com.dw.android.db.model.Entity;

/**
* Helper base class used to do CRUD operations
*
* @param Managed entity class
* @author atancredi
*/
public abstract class EntityHelper {
protected final SQLiteOpenHelper dbHelper;

public EntityHelper(SQLiteOpenHelper dbHelper) {
this.dbHelper = dbHelper;
}

/**
* Load a record from database
*
* @param id Entity id
* @return The entity list loaded from entity table
*/
public T get(long id) {
T retv = null;
//
SQLiteDatabase db = dbHelper.getReadableDatabase();
Cursor cursor = null;
try {
SQLiteQueryBuilder qb= new SQLiteQueryBuilder();
qb.setTables(getTable());
cursor = qb.query(db, getColumns(), "_id = ?", new String[]{ String.valueOf(id) }, null, null, null);
if(cursor.moveToFirst()) {
retv = bind(cursor);
}
} finally {
if(cursor != null) {
cursor.close();
}
db.close();
}
//
return retv;
}

/**
* Load all record from database
* @return The entity list loaded from entity table
*/
public List all() {
List retv = new ArrayList();

I'm not familiar with Android development nor SQLite, so just a few minor generic notes:

-
You have the following method in the EntityHelper:

/**
 * Bind a record to an entity for insert.
 * Remember to not bind the entity id!
 * 
 * @param cursor Cursor from DB
 * @return The binded entity
 */
protected abstract T bind(Cursor cursor);

Despite the javadoc comment the implementation contains an id setting:

/**
 * {@inheritDoc}
 */
protected Alarm bind(Cursor cursor) {
    Alarm alarm = new Alarm();
    //
    alarm.setId(cursor.getLong(Columns._id.ordinal()));
    ...

Are you sure that this is right?

If that's important I'd check it in the EntityHelper and throw an exception if the child class was not implemented properly. (See: The Pragmatic Programmer: From Journeyman to Master by Andrew Hunt and David Thomas: Dead Programs Tell No Lies)

-
You could eliminate the retv variable with two return statements:

if (cursor.moveToFirst()) {
    return bind(cursor);
}
return null;

-
Comments like this are rather noise:

T retv = null;
//
SQLiteDatabase db = dbHelper.getReadableDatabase();

I'd remove them.

-
You could change

if(cursor.moveToFirst()) {
    do {
        retv.add(bind(cursor));
    } while(cursor.moveToNext());
    }

to a simpler while loop:

while (cursor.moveToNext()) {
    retv.add(bind(cursor));
}

-
I usually try to avoid abbreviations like retv. They are not too readable and I suppose you have autocomplete (if not, use an IDE, it helps a lot), so using longer names does not mean more typing but it would help readers and maintainers since they don't have to remember the purpose of each variable - the name would express the programmers intent and doesn't force readers to decode the abbreviations every time they read/maintain the code.

-
I would rename the following method to getTableName():

public abstract String getTable();

It would describe better what it actually does.

StackExchange Code Review Q#23807, answer score: 6