3
\$\begingroup\$

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

}

}
\$\endgroup\$
3
\$\begingroup\$

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.

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

\$\endgroup\$
  • 1
    \$\begingroup\$ 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. \$\endgroup\$ – Nicholas May 12 '12 at 4:41

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.