Problem
I created code for inserting and retrieving data from a specific table. I tried to optimize it and make it as beautiful and easy to read as I can, but maybe (almost certainly) I’m missing something.
It works well, but I wonder if I did all right? Have I forgotten about something?
public class EpgManager {
private DatabaseHandler db;
private List<JSONEpgManagerModel> jsonEpgManagerModel;
private EPGManagerEvent epgManagerEvent;
private SQLiteDatabase sqlDB;
private List<EPGModel> singleEpgChannel;
private List<EPGFullRowModel> fullRowModelList = new ArrayList<>();
public EpgManager(Context context, EPGManagerEvent epgManagerEvent) {
db = new DatabaseHandler(context);
sqlDB = db.getWritableDatabase();
this.epgManagerEvent = epgManagerEvent;
}
/**
* Method: Serializes Json String into Object of type JsonEpgManagementModel
* <ul>
* <li>Creates a sql insert query</li>
* <li>Cleans epg_table</li>
* <li>Loads data from JSON</li>
* <li>Calls the {@link #epgManagerEvent onEpgUpdated} to show completion</li>
* </ul>
*
* @param JSON epg json array
*/
public void updateEpgTable(final String JSON, final HashMap<String, Integer> channelNumberMap,
final HashMap<String, String> channelImageMap) {
new AsyncTask<Void, Void, Void>() {
@Override
protected Void doInBackground(Void... params) {
sqlDB.delete(db.EPG_TABLE, null, null);
try {
jsonEpgManagerModel = Arrays
.asList(new Gson().fromJson(JSON, JSONEpgManagerModel[].class));
} catch (Exception e) {
e.printStackTrace();
}
writeEPGTable(jsonEpgManagerModel, channelNumberMap, channelImageMap);
return null;
}
@Override
protected void onPostExecute(Void aVoid) {
super.onPostExecute(aVoid);
epgManagerEvent.onEpgUpdated();
}
}.execute();
}
/**
* Method: Inserts rows into epg_table
*
* @param jsonEpgManagerModel Serialized EPG Model
* @param channelNumberMap Channel ID and Channel Number HashMap
* @param channelImageMap Channel ID and Channel Image HashMap
*/
private void writeEPGTable(List<JSONEpgManagerModel> jsonEpgManagerModel,
HashMap<String, Integer> channelNumberMap, HashMap<String, String> channelImageMap) {
String epgSqlInsertStatement = "INSERT INTO "
+ db.EPG_TABLE
+ "("
+ db.KEY_EPG_ID + "," + db.KEY_EPG_DATE + ","
+ db.KEY_EPG_DATE_MILLISECONDS + "," + db.KEY_EPG_DISPLAY_TIME + ","
+ db.KEY_EPG_TITLE + "," + db.KEY_EPG_DESCRIPTION + "," + db.KEY_EPG_IMAGE_URL + ","
+ db.KEY_EPG_DURATION + "," + db.KEY_EPG_CHANNEL_ID + ","
+ db.KEY_EPG_CHANNEL_NUMBER + "," + db.KEY_EPG_CHANNEL_IMAGE_LINK
+ ")"
+ " values (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?);";
sqlDB.beginTransaction();
SQLiteStatement sqLiteStatement = sqlDB.compileStatement(epgSqlInsertStatement);
for (int i = 0; i < jsonEpgManagerModel.size(); i++) {
for (int k = 0; k < jsonEpgManagerModel.get(i).getEPGList().size(); k++) {
ChannelPrograms epgProgram = jsonEpgManagerModel.get(i).getEPGList().get(k);
sqLiteStatement.bindString(1, epgProgram.getEpgId());
sqLiteStatement.bindString(2, epgProgram.getEpgDate());
sqLiteStatement.bindLong(3, getDateInMilliseconds(epgProgram.getEpgDate()));
sqLiteStatement.bindString(4, epgProgram.getEpgDisplayTime());
sqLiteStatement.bindString(5, epgProgram.getEpgTitle());
sqLiteStatement.bindString(6, epgProgram.getEpgDescription());
sqLiteStatement.bindString(7, epgProgram.getEpgImageUrl());
sqLiteStatement.bindString(8, epgProgram.getEpgDuration());
sqLiteStatement.bindString(9, jsonEpgManagerModel.get(i).getEpgChannelId());
sqLiteStatement.bindString(10, "" + channelNumberMap.get(jsonEpgManagerModel.get(i).getEpgChannelId()));
sqLiteStatement.bindString(11, "" + channelImageMap.get(jsonEpgManagerModel.get(i).getEpgChannelId()));
sqLiteStatement.executeInsert();
sqLiteStatement.clearBindings();
}
}
sqlDB.setTransactionSuccessful();
sqlDB.endTransaction();
}
/**
* Method returns list of epg channels and programs in order to build the epg
* <ul>
* <li>Gets rows from channel_table</li>
* <li>Gets rows from epg_table by channel_table channel_id</li>
* <li>Populates list of EPGFullRowModel class</li>
* </ul>
*
* @return EPG Screen data list of type EPGFullRowModel
*/
public List<EPGFullRowModel> getAllChannelPrograms() {
List<EPGModel> epgProgramList = new ArrayList<>();
Cursor channelCursor = sqlDB.query(db.CHANNEL_TABLE,
new String[] {db.KEY_CHANNEL_ID, db.KEY_CHANNEL_IMG_LINK, db.KEY_CHANNEL_NUMBER}, null,
null, null, null, null);
Cursor epgCursor;
while (channelCursor.moveToNext()) {
epgCursor = sqlDB.query(db.EPG_TABLE, null, db.KEY_EPG_CHANNEL_ID + "=?",
new String[] {channelCursor.getString(0)}, null, null,
db.KEY_EPG_DATE_MILLISECONDS + " ASC");
while (epgCursor.moveToNext()) {
epgProgramList.add(
new EPGModel(
epgCursor.getString(1),epgCursor.getString(2),
epgCursor.getString(3),epgCursor.getString(4),
epgCursor.getString(5),epgCursor.getString(6),
epgCursor.getString(7),epgCursor.getString(8),
epgCursor.getString(9),channelCursor.getString(2),
epgCursor.getString(1)
)
);
}
fullRowModelList.add(new EPGFullRowModel(
new ChannelModel(channelCursor.getString(0), channelCursor.getString(1),
Integer.valueOf(channelCursor.getString(2))), epgProgramList));
epgCursor.close();
epgProgramList = new ArrayList<>();
}
channelCursor.close();
return fullRowModelList;
}
/**
* Method returns list of epg programs in order to build the single channel epg
*
* @param channelId Channel unique id
* @return Single epg row list of type EPGModel
*/
public List<EPGModel> getSingleChannelPrograms(String channelId) {
Cursor cursor = sqlDB
.query(db.EPG_TABLE, null, db.KEY_EPG_CHANNEL_ID + "=?", new String[] {channelId}, null,
null, db.KEY_EPG_DATE_MILLISECONDS + " ASC", null);
singleEpgChannel = new ArrayList<>();
Log.e("CURSOR ", String.valueOf(cursor.getCount()));
if (cursor.moveToFirst()) {
do {
singleEpgChannel.add(
new EPGModel(cursor.getString(1), cursor.getString(2), cursor.getString(3),
cursor.getString(4), cursor.getString(5), cursor.getString(6),
cursor.getString(7), cursor.getString(8), cursor.getString(9),
cursor.getString(10), cursor.getString(11)));
} while (cursor.moveToNext());
}
cursor.close();
return singleEpgChannel;
}
/**
* Method transforms the date of string into long of milliseconds
*
* @param date Epg program date
* @return Milliseconds
*/
private long getDateInMilliseconds(String date) {
date = date.replaceAll("T", " ");
date = date.replaceAll("Z", "");
SimpleDateFormat simpleDateFormat = new SimpleDateFormat("yyyy-MM-dd HH:mm:ss");
Calendar calendar = Calendar.getInstance();
try {
calendar.setTime(simpleDateFormat.parse(date));
calendar.set(Calendar.SECOND, 0);
calendar.set(Calendar.MILLISECOND, 0);
} catch (ParseException e) {
e.printStackTrace();
}
return calendar.getTimeInMillis();
}
}
Solution
As RobAu said, there’s a lot going on. A priori, I am developing enterprise applications, so there’s usually a lot of stupid logic involved, way too much data and thousands of concurrent acccesses to databases – and maybe I sound like a bit like I’m ranting and nitpicking and maybe some suggestions are a bit overkill. But since we’re here…
Transactions / unit of works
You are working with transactions within the writeEGPTable method. But this method is called in updateEpgTable which actually does whipe the table. Just keep in mind, that if you have a unit of work, where you do more than an insert, you can get inconsistent data in a concurrent environment (not only concurrent within your app, but others accessing and manipulating the database). The transaction management is usually decoupled from the database access object, since, like in your example, the writeEPGTable method does not know about what is happening around it. I highly recommend to read about those topics (transaction management, unit of work, isolation levels). Speaking from experience: It’s surprisingly easy to mess everything up, if you do not know exactly what you are doing. So easy …
delete and insert all
Usually, deleting and inserting is way more expensive than to update existing, insert new ones, and deleting non existing ones. If you delete everything, and insert everything, you mess up indexes and statistics.
column index access
During reading, you read the values using the column indexes. It’s almost always better to use the column names. If you declare the columns’ order within select statement (which I do not recommend), you always have to change both, the statement and the retrieving of the column values. If you select * and the columns get reordered, your application will break.
JSON / Model
You actually provide an EPGFullRowModel, but you pass json to updateEpgTable, which then is de-jsonified to the an List<EpgManagerModel>, which then will be passed to writeEPGTable, within that, you get the ChannelPrograms and create the insert statement. The json has no business in the data layer, the mapping from json to the actual type to write should be performed in its separate type. I recommend to write a type which holds the data it needs and pass this one, instead of JSON, a map of channelNumbers and a map of channelImages.
tidy up
Your methods are way too long. For instance, the writeEPGTable, consider the following points: Declare the insert statement as constant in the class and move the binding of the values to a separate static helper method. That should look much better.
SQLite
I never worked with SQLite, to be honest. But: Keep in mind, you cannot exchange your database without a huge amount of work – at least that’s how it looks. I would have implemented it using standard jdbc, assuming sqllite provides a jdbc driver.
other
- I think there’s code duplication for the instantiation of ChannelModel in
getAllChannelProgramsandgetSingleChannelPrograms - You declare
singleEpgChannelas member variable, but it’s only instantiated and accessed ingetSingleChannelPrograms, which does return the actual membersingleEpgChannel. The member variable can be removed.
further readings
I made this section, because you want to write your own database access, and to not use ORM’s like hibernate or eclipse’s link. So this should interest you
Maybe you’d like to check Fowler’s list of eea patterns: https://martinfowler.com/eaaCatalog/ – especially the Data Source Architectural Patterns. And also the “dao” j2ee pattern from core j2ee patterns: http://www.corej2eepatterns.com/DataAccessObject.htm, that’s a very common one (which only decouples backend implementation, there’s still a lot of other problems).
Also, “Tom” from oracle has a lot of very, like very good blog posts about databases, not only oracle specific. Just google “ask tom ” + whatever you want to know (about databases, of course). For transaction management and isolation levels, read this one: http://www.oracle.com/technetwork/testcontent/o65asktom-082389.html.
(Keep in mind, design patterns can’t usually work on their own and will not per se solve your problems. But it should give you an idea, how problems have been solved and you can apply that to your own design).
Hops this helps.
There is a lot going on in your code, but this struck me first:
If / do / while
if (cursor.moveToFirst()) {
do {
.....
} while (cursor.moveToNext());
}
This can be written as:
while (cursor.moveToFirst()) {
.....
}
Static / Casing?
This is something you should never see:
db.EPG_TABLE
Either EPG_TABLE is static on DatabaseHandler, in which case you should access it using DatabaseHandler.EPG_TABLE. Or it is a field, and then it should be camelCase.