SQLiteOpenHelper for multi string tables

Question

I wrote this simple SQLiteOpenHelper in order to use in one database various tables containing String records (each record having a String identifier, and associated String fields.

When you create a table, you have to call the createTable method with a table name and its number of fields. Table fields are then created internally as _id, ID, DATA1, DATA2, DATA3, ... according to the number of fields.

_id field (INTEGER) is the real primary key, but invisible for the user, who doesn't have to worry about it.

ID field (TEXT) is the visible primary key for the user (with a UNIQUE constraint on it).

Of course, the user of the class has to keep track of the indexes and the ID field for each record, for example with some ENUMs. He never has to provide the _id field or space for it.

If the data contains other data types than String, you can also convert it to String before inserting new records and convert it back after reading records.

What do you think of it?

package com.example.bonux.pekislib_a;

import android.content.Context;
import android.database.Cursor;
import android.database.sqlite.SQLiteDatabase;
import android.database.sqlite.SQLiteOpenHelper;

import java.util.Arrays;

//  Table fields are always _id, ID, DATA1, DATA2, DATA3, ... according to the number of fields specified at createTable
//  _id field is the real primary key, but invisible for the user
//  ID field is the visible primary key for the user (see UNIQUE constraint on ID field at createTable)
public class StringShelfDatabase extends SQLiteOpenHelper {

    //region Constantes
    private enum FIELDS {
        _id(0), ID(1), DATA(2);

        private int valueIndex;

        FIELDS(int valueIndex) {
            this.valueIndex = valueIndex;
        }

        public int INDEX() {
            return valueIndex;
        }

        public int USER_INDEX() {
            return INDEX() - 1;
        }    //  visible Index for the user
    }

    private static final int DATABASE_VERSION = 5;
    private static final String DATABASE_NAME = "ssDB";
    private final String NULL_STRING = "NULL@" + DATABASE_NAME;   //  String to use in case of null field
    //endregion

    public StringShelfDatabase(Context context) {
        super(context, DATABASE_NAME, null, DATABASE_VERSION);
    }

    @Override
    public void onCreate(SQLiteDatabase database) {  //  Tables are created only after calling createTable
        //  NOP
    }

    @Override
    public void onUpgrade(SQLiteDatabase database, int oldVersion, int newVersion) {
        //  NOP
    }

    public int getIdIndex() {
        return FIELDS.ID.USER_INDEX();
    }

    public String getFieldName(int fieldIndex) {
        String ret;

        if (fieldIndex == FIELDS.ID.USER_INDEX()) {
            ret = FIELDS.ID.toString();
        } else {
            ret = FIELDS.DATA.toString() + String.valueOf(fieldIndex);
        }
        return ret;
    }

    public boolean tableExists(String tableName) {
        final int COUNT_INDEX = 0;
        SQLiteDatabase database;
        Cursor cursor;
        boolean ret;

        database = this.getReadableDatabase();
        cursor = database.rawQuery(sqlForTableExists(tableName), null);
        cursor.moveToFirst();    //  Never null cursor after rawQuery using sqlForTableExists
        ret = (cursor.getInt(COUNT_INDEX) > 0);
        cursor.close();
        cursor = null;
        database.close();
        database = null;
        return ret;
    }

    public void createTable(String tableName, int tableFieldsCount) {
        SQLiteDatabase database;

        database = this.getWritableDatabase();
        database.execSQL(sqlForCreateTable(tableName, tableFieldsCount));
        database.close();
        database = null;
    }

    public String[][] selectRows(String tableName, String whereCondition) {
        SQLiteDatabase database;
        Cursor cursor;
        String[][] ret;
        int rowCount;
        int columnCount;
        String fieldValue;
        int i;
        int j;

        ret = null;
        database = this.getReadableDatabase();
        cursor = database.rawQuery(sqlForSelectRows(tableName, whereCondition), null);
        if (cursor != null) {
            rowCount = cursor.getCount();
            if (rowCount > 0) {
                cursor.moveToFirst();
                columnCount = cursor.getColumnCount();
                ret = new String[rowCount][columnCount - 1];
                for (i = 0; i <= (rowCount - 1); i = i + 1) {
                    for (j = FIELDS.ID.INDEX(); j <= (columnCount - 1); j = j + 1) {   //  All fields except _id
                        fieldValue = cursor.getString(j);
                        if (fieldValue.equals(NULL_STRING)) {
                            fieldValue = null;
                        }
                        ret[i][j - 1] = fieldValue;
                    }
                    cursor.moveToNext();
                }
            }
            cursor.close();
            cursor = null;
        }
        database.close();
        database = null;
        return ret;
    }

    public String[] selectRowById(String tableName, String idValue) {
        String[][] stsa;
        String[] ret;

        stsa = selectRows(tableName, FIELDS.ID.toString() + " = '" + idValue + "'");
        if (stsa != null) {
            ret = stsa[0];    //  One record only (see UNIQUE constraint on ID field)
            stsa = null;
        } else {    //  IdValue unknown in the table => Create an empty record in the table, with that IdValue
            ret = new String[getTableFieldsCount(tableName) - 1];   //  Don't count _id field
            ret[FIELDS.ID.USER_INDEX()] = idValue;
            insertOrReplaceRow(tableName, ret);
        }
        return ret;
    }

    public String selectFieldById(String tableName, String idValue, int fieldIndex) {
        return selectRowById(tableName, idValue)[fieldIndex];
    }

    public void insertOrReplaceRows(String tableName, String[][] rows) {
        SQLiteDatabase database;
        int i;

        database = this.getWritableDatabase();
        if (rows != null) {
            for (i = 0; i <= (rows.length - 1); i = i + 1) {
                database.execSQL(sqlForInsertOrReplaceUserRow(tableName, rows[i]));
            }
        }
        database.close();
        database = null;
    }

    public void insertOrReplaceRow(String tableName, String[] row) {
        SQLiteDatabase database;

        database = this.getWritableDatabase();
        database.execSQL(sqlForInsertOrReplaceUserRow(tableName, row));
        database.close();
        database = null;
    }

    public void insertOrReplaceRowById(String tableName, String idValue, String[] row) {
        String[] sts;

        sts = Arrays.copyOfRange(row, 0, row.length);
        sts[FIELDS.ID.USER_INDEX()] = idValue;
        insertOrReplaceRow(tableName, sts);
        sts = null;
    }

    public void insertOrReplaceFieldById(String tableName, String idValue, int fieldIndex, String value) {
        String[] sts;

        sts = selectRowById(tableName, idValue);
        sts[fieldIndex] = value;
        insertOrReplaceRow(tableName, sts);
        sts = null;
    }

    public void deleteRows(String tableName, String whereCondition) {
        SQLiteDatabase database;

        database = this.getWritableDatabase();
        database.execSQL(sqlForDeleteRows(tableName, whereCondition));
        database.close();
        database = null;
    }

    private int getTableFieldsCount(String tableName) {
        SQLiteDatabase database;
        Cursor cursor;
        int ret;

        database = this.getReadableDatabase();
        cursor = database.rawQuery(sqlForGetTableFieldsCount(tableName), null);
        ret = cursor.getColumnNames().length;    //  Never null cursor after using sqlForGetTableFieldsCount in rawQuery
        cursor.close();
        cursor = null;
        database.close();
        database = null;
        return ret;
    }

    private String sqlForCreateTable(String tableName, int tableUserFieldsCount) {
        String ret;
        String fieldNamesAndTypes;
        int j;

        fieldNamesAndTypes = FIELDS._id.toString() + " INTEGER PRIMARY KEY, ";   //  _id field
        fieldNamesAndTypes = fieldNamesAndTypes + FIELDS.ID.toString() + " TEXT NOT NULL UNIQUE, ";   //  ID field
        for (j = 1; j <= (tableUserFieldsCount - 1); j = j + 1) {
            fieldNamesAndTypes = fieldNamesAndTypes + FIELDS.DATA.toString() + String.valueOf(j) + " TEXT";    //  Fields DATA1, DATA2, DATA3, ...
            if (j < (tableUserFieldsCount - 1)) {
                fieldNamesAndTypes = fieldNamesAndTypes + ", ";
            }
        }
        ret = "CREATE TABLE " + tableName + " ( " + fieldNamesAndTypes + " )";
        return ret;
    }

    private String sqlForSelectRows(String tableName, String whereCondition) {
        String ret;

        ret = "SELECT * FROM " + tableName;
        if (whereCondition != null) {
            ret = ret + " WHERE " + whereCondition;
        }
        return ret;
    }

    private String sqlForInsertOrReplaceUserRow(String tableName, String[] userRow) {   //  Works because of UNIQUE constraint on ID field
        String ret;
        String fieldNames;
        String fieldValues;
        String fieldValue;
        int j;

        fieldNames = FIELDS.ID.toString() + ", ";    //   ID field (No need to worry about _id field)
        fieldValues = "'" + userRow[FIELDS.ID.USER_INDEX()] + "', ";
        for (j = FIELDS.DATA.USER_INDEX(); j <= (userRow.length - 1); j = j + 1) {    //  Fields DATA1, DATA2, DATA3, ...
            fieldNames = fieldNames + FIELDS.DATA.toString() + String.valueOf(j);
            fieldValue = userRow[j];
            if (fieldValue == null) {
                fieldValue = NULL_STRING;
            }
            fieldValues = fieldValues + "'" + fieldValue + "'";
            if (j < (userRow.length - 1)) {
                fieldNames = fieldNames + ", ";
                fieldValues = fieldValues + ", ";
            }
        }
        ret = "INSERT OR REPLACE INTO " + tableName + " ( " + fieldNames + " )" + " VALUES (" + fieldValues + ")";
        return ret;
    }

    private String sqlForDeleteRows(String tableName, String whereCondition) {
        String ret;

        ret = "DELETE FROM " + tableName;
        if (whereCondition != null) {
            ret = ret + " WHERE " + whereCondition;
        }
        return ret;
    }

    private String sqlForTableExists(String tableName) {
        return "SELECT COUNT(*) FROM sqlite_master WHERE ((name = '" + tableName + "') AND (type = 'table'))";   //  Never null cursor after using this SQL in rawQuery
    }

    private String sqlForGetTableFieldsCount(String tableName) {
        return "SELECT * FROM " + tableName + " LIMIT 0";    //  Never null cursor after using this SQL in rawQuery
    }

}

Answer 1

Prevent SQL injection attacks, always

The implementation is susceptible to SQL injection attacks, because all parameterized queries use string concatenation to construct SQL. Even if you know that all callers can be trusted, it would be better to use prepared statements to give a chance to the database to perform optimizations.

Performance issues

All methods that execute some query close the database immediately. Keep in mind the relevant documentation:

Since getWritableDatabase() and getReadableDatabase() are expensive to call when the database is closed, you should leave your database connection open for as long as you possibly need to access it. Typically, it is optimal to close the database in the onDestroy() of the calling Activity.

If the application will perform multiple database operations using this class, then it will be better to let it call .close() when it's done. This is a very likely scenario, and as the documentation says, probably it's best to close connections in onDestroy() of the Activity.

---

It's not recommended to do wildcard selects such as SELECT * FROM .... It may select more columns than you actually need (performance issue), and it may select different columns than you expect (functional issue). It's best to name the columns you want to select, and then you will get back precisely those columns, in the specified order.

---

Not a very real one, but a potential performance issue: selectRowById calls selectRows and then returns the first of the returned records. I understand that you expect at most one record returned by selectRows in this case, thanks to the unique constraint, but this knowledge is too implicit. Programming errors do happen. The unique constraint you assume to be there might not be. The ID value parameter might be a malicious value such as ' OR '' = ' to force selecting all records. Avoid implicit rules that require knowledge outside a method, when possible. For example you could add a limit parameter to selectRows, and make selectRowById pass 1 as the value. The method will be much more robust, requiring less knowledge about code outside of it.

Avoid unexpected side effects

selectRowById has a side effect that's unexpected: if there is no matching record, it inserts one. A common practice is to call such method getOrCreate.

Usability

If the data contains other data types than String, you can also convert it to String before inserting new records and convert it back after reading records.

Sure. At a cost of performance. And you won't be able to do numeric queries on the numeric data. In reality, this helper class will not be practical for non-string data. I would discourage such uses, and make it perfectly clear that this is strictly for string data only.

Minimize the live time of variables

It's best to declare variables in the smallest possible scope. Don't declare all variables of a method at the top of the method. Declare right before you need them. Whenever possible, it's best to declare variables when you initialize them.

That is, instead of this:

public String[][] selectRows(String tableName, String whereCondition) {
    SQLiteDatabase database;
    Cursor cursor;
    String[][] ret;
    int rowCount;
    int columnCount;
    String fieldValue;
    int i;
    int j;

    ret = null;
    database = this.getReadableDatabase();
    cursor = database.rawQuery(sqlForSelectRows(tableName, whereCondition), null);
    if (cursor != null) {
        rowCount = cursor.getCount();
        if (rowCount > 0) {
            cursor.moveToFirst();
            columnCount = cursor.getColumnCount();
            ret = new String[rowCount][columnCount - 1];
            for (i = 0; i <= (rowCount - 1); i = i + 1) {
                for (j = FIELDS.ID.INDEX(); j <= (columnCount - 1); j = j + 1) {   //  All fields except _id
                    fieldValue = cursor.getString(j);
                    if (fieldValue.equals(NULL_STRING)) {
                        fieldValue = null;
                    }
                    ret[i][j - 1] = fieldValue;
                }
                cursor.moveToNext();
            }
        }
        cursor.close();
        cursor = null;
    }
    database.close();
    database = null;
    return ret;
}

Write like this:

public String[][] selectRows(String tableName, String whereCondition) {
    SQLiteDatabase database = this.getReadableDatabase();
    Cursor cursor = database.rawQuery(sqlForSelectRows(tableName, whereCondition), null);
    if (cursor != null) {
        String[][] ret;
        int rowCount = cursor.getCount();
        if (rowCount > 0) {
            cursor.moveToFirst();
            int columnCount = cursor.getColumnCount();
            ret = new String[rowCount][columnCount - 1];
            for (int i = 0; i < rowCount; i++) {
                for (int j = FIELDS.ID.INDEX(); j < columnCount; j++) {   //  All fields except _id
                    String fieldValue = cursor.getString(j);
                    if (fieldValue.equals(NULL_STRING)) {
                        fieldValue = null;
                    }
                    ret[i][j - 1] = fieldValue;
                }
                cursor.moveToNext();
            }
        }
        cursor.close();
        return ret;
    }
    database.close();
    return ret;
}

While writing this, I noticed a code smell:

ret = new String[rowCount][columnCount - 1];
for (int i = 0; i < rowCount; i++) {
    for (int j = FIELDS.ID.INDEX(); j < columnCount; j++) {   //  All fields except _id
        // ...
        ret[i][j - 1] = ...
    }
    // ...
}

The suspicious point is the number of columns in ret. The above code works perfectly when FIELDS.ID.INDEX() is 1. If that value is higher, then ret will use more memory than it should. If that value is 0, which doesn't seem too crazy and might happen with a small programming error, the program will crash, trying to assign to an index out of bounds.

It seems to me the least of all evils here would be to change the loop to use int j = 1. Then the code smell will disappear, and the program will never crash. But it might not exclude the correct column.

If you know that precisely one of the columns should be excluded, then it would be more robust to not depend on its index. Change the loop to iterate over all columns, and add a condition inside the loop body to skip over the column to be excluded.

It would be best to not select at all any columns that you don't need in the result.

Minor technical issues

You don't need to null out local variables like this.

database.close();
database = null;

It's enough to close them, if you really need to close them.

---

No need to call String.valueOf when part of a string concatenation, for example in FIELDS.DATA.toString() + String.valueOf(fieldIndex);. The compiler will do that for you automatically.

---

You could shorten this by dropping an unnecessary local variable and using early returns:

public String getFieldName(int fieldIndex) {
    String ret;

    if (fieldIndex == FIELDS.ID.USER_INDEX()) {
        ret = FIELDS.ID.toString();
    } else {
        ret = FIELDS.DATA.toString() + String.valueOf(fieldIndex);
    }
    return ret;
}

Like this:

public String getFieldName(int fieldIndex) {
    if (fieldIndex == FIELDS.ID.USER_INDEX()) {
        ret = FIELDS.ID.toString();
    }

    return FIELDS.DATA.toString() + fieldIndex;
}