AsyncTask, Android, and SQL

Question

I originally posted this in stackoverflow.com but the question may be too broad. I'm trying to figure out the best way to download and use an SQL database from my server. I have included the code i whipped up but I'm not sure if it's a viable way to accomplish this so peer review would be extremely helpful :)

As it stands now, the database will be downloaded in a separate thread but when UI components are initialized they fail (obviously, as the database doesnt exist while its still being downloaded).

package com.sandbox.databaseone;
import java.io.BufferedReader;
import java.io.FileOutputStream;
import java.io.InputStream;
import java.io.InputStreamReader;
import org.apache.http.HttpEntity;
import org.apache.http.HttpResponse;
import org.apache.http.client.HttpClient;
import org.apache.http.client.methods.HttpGet;
import org.apache.http.impl.client.DefaultHttpClient;
import android.database.sqlite.SQLiteDatabase;
import android.os.AsyncTask;
import android.util.Log;
public class DatabaseManager {
private SQLiteDatabase database;
private int currentVersion = 0;
private int nextVersion = 0;
private static String databasePath = "/data/data/com.sandbox.databaseone/databases/";
private static String databaseFile = "dbone.sqlite";
private static String databaseBaseURL = "http://www.redstalker.com/dbone/";
private static String databaseVersionURL = "version.txt";
public DatabaseManager()
{
 database = null;
}
public void initialize()
{
 DatabaseVersionCheck check = new DatabaseVersionCheck();
 String url = databaseBaseURL + databaseVersionURL;
 check.execute(url);
}
private void init_database(String path)
{
 database = SQLiteDatabase.openDatabase(path, null, SQLiteDatabase.OPEN_READONLY);
 if(database != null)
 {
 currentVersion = nextVersion;
 }
 else
 {
 nextVersion = 0;
 }
}
private class DatabaseVersionCheck extends AsyncTask<String, Void, String>
{
 @Override
 protected String doInBackground(String... params) {
 StringBuilder builder = new StringBuilder();
 HttpClient client = new DefaultHttpClient();
 HttpGet get = new HttpGet(params[0]);
 try
 {
 HttpResponse response = client.execute(get);
 int statusCode = response.getStatusLine().getStatusCode();
 if(statusCode == 200)
 {
 HttpEntity entity = response.getEntity();
 InputStream in = entity.getContent();
 BufferedReader reader = new BufferedReader(new InputStreamReader(in));
 String line;
 while((line = reader.readLine()) != null)
 {
 builder.append(line);
 }
 in.close();
 reader.close();
 entity.consumeContent();
 }
 } 
 catch(Exception e)
 {
 e.printStackTrace();
 }
 return builder.toString();
 }
 @Override
 protected void onPostExecute(String result)
 {
 if(result != null)
 {
 int version = Integer.parseInt(result);
 if(version > currentVersion)
 {
 nextVersion = version;
 DownloadDatabase d = new DownloadDatabase();
 d.execute();
 }
 }
 }
}
private class DownloadDatabase extends AsyncTask<Void, Void, Boolean>
{
 @Override
 protected Boolean doInBackground(Void... params) {
 boolean result = false;
 String url = databaseBaseURL + databaseFile;
 String path = databasePath + databaseFile;
 HttpClient client = new DefaultHttpClient();
 HttpGet get = new HttpGet(url);
 try
 {
 HttpResponse response = client.execute(get);
 int statusCode = response.getStatusLine().getStatusCode();
 if(statusCode == 200)
 {
 FileOutputStream fos = new FileOutputStream(path);
 HttpEntity entity = response.getEntity();
 InputStream in = entity.getContent();
 byte [] buffer = new byte[1024];
 int count = 0;
 while((count = in.read(buffer)) != -1)
 {
 fos.write(buffer, 0, count);
 }
 fos.close();
 in.close();
 entity.consumeContent();
 result = true;
 }
 }
 catch(Exception e)
 {
 e.printStackTrace();
 }
 return result;
 }
 @Override
 protected void onPostExecute(Boolean result)
 {
 String path = databasePath + databaseFile;
 if(result)
 {
 init_database(path);
 }
 }
}
}

Answer 1

Some generic Java notes, since I'm not too familiar with Android.

1. databasePath, databaseFile, databaseBaseURL, databaseVersionURL should be constants (all uppercase with words separated by underscores):

   private static final String DATABASE_PATH = 
    "/data/data/com.sandbox.databaseone/databases/";
   private static final String DATABASE_FILE = "dbone.sqlite";
   private static final String DATABASE_BASE_URL = 
    "http://www.redstalker.com/dbone/";
   private static final String DATABASE_VERSION_URL =
    "version.txt";
   

   Reference: Code Conventions for the Java Programming Language, 9 - Naming Conventions

2. According to the previous Code Conventions, init_database should be initDatabase.

3. If databaseBaseURL and databaseVersionURL hadn't be constant I'd named them as databaseBaseUrl and databaseBaseVersionUrl. From Effective Java, 2nd edition, Item 56: Adhere to generally accepted naming conventions:

   While uppercase may be more common, a strong argument can made in favor of capitalizing only the first letter: even if multiple acronyms occur back-to-back, you can still tell where one word starts and the next word ends. Which class name would you rather see, HTTPURL or HttpUrl?

4. public DatabaseManager()
   {
    database = null;
   }
   

   Initializing fields with null is unnecessary since null is the default value of references.

5. I'd call the StringBuilder in the doInBackground method as result.

   StringBuilder builder = new StringBuilder();
   

   It would say what's the purpose of the object. For the same reason I'd rename the

   • boolean result to boolean success,
   • in to responseStream, and
   • d to downloadDatabase.

6. Close your streams in a finally block. In case of a previous errors they won't be closed.

7. The code does databasePath + databaseFile more than once. Create a method for that.

8. I don't know if it is applicable to Android or not, but in Java it's a good practice to pass the character set to the constructor of InputStreamReader. Without this InputStreamReader uses the default charset which could vary from system to system.

   BufferedReader reader = 
    new BufferedReader(new InputStreamReader(in, "UTF-8"));
   

9. I'd use Commons IO's IOUtils for the copying. It has a copy(InputStream input, OutputStream output) which you could use in the DownloadDatabase task and you could use copy(InputStream input, Writer output) in the DatabaseVersionCheck task if you replace the StringBuilder to a StringWriter.

10. In the DatabaseVersionCheck.onPostExecute I'd use guard clauses:

    @Override
    protected void onPostExecute(String result) {
     if(result == null) {
     return;
     }
     int version = Integer.parseInt(result);
     if(version <= currentVersion) {
     return;
     }
     nextVersion = version;
     DownloadDatabase d = new DownloadDatabase();
     d.execute();
    }
    

    It makes the code flatten and more readable. References: Replace Nested Conditional with Guard Clauses in Refactoring: Improving the Design of Existing Code; Flattening Arrow Code

11. e.printStackTrace() is not the best practice. Maybe you should inform the user about the error and/or log it.

    • Why is exception.printStackTrace() considered bad practice?
    • Is it a bad idea to use printStackTrace() in Android Exceptions?

12. Integer.parseInt could throw NumberFormatException. Maybe the code should handle it.

• Nicholas: Wow... you really just schooled me in Java. I will definitely take this to heart and study up to improve, I appreciate it. Hopefully my next version will be cleaner.