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.

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?

share|improve this question
    
avoid using enums in Android, use static final fields. –  Marco Acierno Apr 4 '14 at 16:12

2 Answers 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

share|improve this answer

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.

share|improve this answer

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.