Android database access

Question

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 <T> Managed entity class
 * @author atancredi
 */
public abstract class EntityHelper<T extends Entity> {
    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<T> all() {
        List<T> retv = new ArrayList<T>();
        //
        SQLiteDatabase db = dbHelper.getReadableDatabase();
        SQLiteQueryBuilder qb= new SQLiteQueryBuilder();
        qb.setTables(getTable());
        Cursor cursor = null;
        try {
            cursor = qb.query(db, getColumns(), null, null, null, null, null);
            if(cursor.moveToFirst()) {
                do {
                    retv.add(bind(cursor));
                } while(cursor.moveToNext());
            }
        } finally {
            if(cursor != null) {
                cursor.close();
            }
            db.close();
        }
        //
        return retv;
    }

    /**
     * Update an entity on DB using his id as selection
     * 
     * @param entity Entity to update
     * @return true, if a row has been update
     */
    public boolean update(T entity) {
        SQLiteDatabase db = dbHelper.getWritableDatabase();
        boolean updated = true;
        try {
            db.beginTransaction();
            int count = db.update(getTable(), bind(entity), "_id = ?", new String[]{ String.valueOf(entity.getId()) });
            updated = count > 0;
            db.setTransactionSuccessful();
        } finally {
            db.endTransaction();
            db.close();
        }
        return updated;
    }

    /**
     * Insert an entity on the DB
     * 
     * @param entity Entity to insert
     * @return The DB generated id for the entity
     */
    public long insert(T entity) {
        SQLiteDatabase db = dbHelper.getWritableDatabase();
        long retv = -1;
        try {
            db.beginTransaction();
            retv = db.insert(getTable(), null, bind(entity));
            db.setTransactionSuccessful();
            if(retv >= 0) {
                entity.setId(retv);
            }
        } finally {
            db.endTransaction();
            db.close();
        }
        return retv;
    }

    /**
     * Delete an entity
     * @param id Entity id
     * @return true, if the entity was in the DB
     */

    public boolean delete(T entity) {
        return delete(entity.getId());
    }

    /**
     * Delete an entity
     * @param id Entity id
     * @return true, if the entity was in the DB
     */
    public boolean delete(long id) {
        SQLiteDatabase db = dbHelper.getWritableDatabase();
        boolean deleted = false;
        try {
            db.beginTransaction();
            int count = db.delete(getTable(), "_id = ?", new String[]{ String.valueOf(id) });
            deleted = count > 0;
            db.setTransactionSuccessful();
        } finally {
            db.endTransaction();
            db.close();
        }
        return deleted;
    }

    /**
     * Build the columns array using an enum
     * 
     * @return The columns array, that can be used for a projection
     */
    protected String[] getColumns() {
        Enum<?>[] columns = getColumnsEnum();
        String[] retv  = new String[columns.length];
        for(int i = 0, len = columns.length; i < len; i++) {
            retv[i] = columns[i].name();
        }
        return retv;
    }

    /**
     * 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);

    /**
     * Bind an entity to a ContentValues
     * 
     * @param entity The entity
     * @return A ContentValues object that contains the record values
     */
    protected abstract ContentValues bind(T entity);

    /**
     * Get the table name for the enttiy
     * 
     * @return The table name
     */
    public abstract String getTable();

    /**
     * Get the enum that define all columns for the entity table
     * 
     * @return The enum values
     */
    public abstract Enum<?>[] getColumnsEnum();

}

This is an example of class that extends EntityHelper:

package com.dw.svegliatest.db.model;

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

import com.dw.android.db.EntityHelper;
import com.dw.utils.time.Days;

/**
 * Alarm entity helper
 * 
 * @author atancredi
 */
public final class AlarmHelper extends EntityHelper<Alarm> {
    public AlarmHelper(SQLiteOpenHelper dbHelper) {
        super(dbHelper);
    }

    /**
     * {@inheritDoc}
     */
    protected ContentValues bind(Alarm entity) {
        ContentValues record = new ContentValues();
        //
        record.put(Columns.label.name(), entity.getLabel());
        record.put(Columns.enabled.name(), entity.isEnabled());
        record.put(Columns.time.name(), entity.getTime());
        record.put(Columns.days.name(), entity.getDays().flags());
        //
        return record;
    }

    /**
     * {@inheritDoc}
     */
    protected Alarm bind(Cursor cursor) {
        Alarm alarm = new Alarm();
        //
        alarm.setId(cursor.getLong(Columns._id.ordinal()));
        alarm.setLabel(cursor.getString(Columns.label.ordinal()));
        alarm.setEnabled(cursor.getInt(Columns.enabled.ordinal()) == 1);
        alarm.setTime(cursor.getLong(Columns.time.ordinal()));
        alarm.setDays(new Days(cursor.getInt(Columns.days.ordinal())));
        //
        return alarm;
    }

    /**
     * {@inheritDoc}
     */
    public String getTable() {
        return "Alarm";
    }

    /**
     * {@inheritDoc}
     */
    public Enum<?>[] getColumnsEnum() {
        return Columns.values();
    }

    /**
     * Alarm columns definition
     * 
     * @author atancredi
     */
    public static enum Columns {
        _id,
        label,
        enabled,
        time,
        days
    }
}

What do you think about the code and the idea of use an enum for table columns?

Answer 1

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

1. 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)

2. You could eliminate the retv variable with two return statements:

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

3. Comments like this are rather noise:

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

   I'd remove them.

4. 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));
   }
   

5. 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.

6. I would rename the following method to getTableName():

   public abstract String getTable();
   

   It would describe better what it actually does.

Answer 2

I think inheritance is unnecessary here. It has some drawbacks:

• AlarmHelper does not use the SQLiteOpenHelper just passes it to the superclass. This is a superfluous dependency.
• If you want to add a new constructor parameter to EntityHelper (or modify an existing one) you have to change every subclass.
• It's hard to test the logic in AlarmHelper.

I would go with composition.

First, a new EntityMapper interface:

public interface EntityMapper<T extends Entity> {
    T bind(Cursor cursor);
    ContentValues bind(T entity);
    String getTable();
    Enum<?>[] getColumnsEnum();
}

(You might be able to find a better name.)

Then an AlarmMapper (it's methods are the same as AlarmHelper):

public final class AlarmMapper implements EntityMapper<Alarm> {
    public ContentValues bind(Alarm entity) {
        ...
    }

    public Alarm bind(Cursor cursor) {
        ...
    }

    public String getTable() {
        ...
    }

    public Enum<?>[] getColumnsEnum() {
        ...
    }
}

Here is the modified EntityHelper:

public class EntityHelper<T extends Entity> {
    private final SQLiteOpenHelper dbHelper;
    private final EntityMapper<T> entityMapper;

    public EntityHelper(SQLiteOpenHelper dbHelper, EntityMapper<T> entityMapper) {
        this.dbHelper = dbHelper;
        this.entityMapper = entityMapper;
    }

    ...

    public boolean delete(long id) {
        ...
        int count = db.delete(entityMapper.getTable(), 
            "_id = ?", new String[] { String.valueOf(id) });
        ...
    }

    ...
}

Finally, you can create a factory which is the only place where EntityHelper is created and the only place where it has to be changed if it gets a new dependency:

public class EntityHelperFactory {

    private final SQLiteOpenHelper dbHelper;

    public EntityHelperFactory(SQLiteOpenHelper dbHelper) {
        this.dbHelper = checkNotNull(dbHelper, "dbHelper cannot be null");
    }

    public <T extends Entity> EntityHelper<T> create(EntityMapper<T> entityMapper) {
        return new EntityHelper<T>(dbHelper, entityMapper);
    }
}

See also: Effective Java, 2nd Edition, Item 16: Favor composition over inheritance