Downloading a page in Android

Question

I tried to create a class which downloads the HTML source from a link and saves it. I'm a newbie to all this Android stuff, even new to Java, but this is what I have so far.

It seems to work at least, although I cannot see the file since it is hidden.

package com.example.test;

import java.io.File;
import java.io.FileNotFoundException;
import java.io.FileOutputStream;
import java.io.IOException;
import java.io.InputStream;
import java.net.MalformedURLException;
import java.net.URL;
import java.net.URLConnection;
import android.content.Context;
import android.os.AsyncTask;
import android.util.Log;

public class PageDownloader {

    String Filename;
    String urlString;
    Context context;


    public PageDownloader( Context ctx) {
        Filename = "tmpPage.html";
        context = ctx;
    }

    public void download( String _url ){
        urlString = _url;
        testClass t = new testClass();
        t.execute();

    }
    public void _download(){
        /*
         * First Download content
         * Then Save File
         * 
         */
        String result = null;

        //DOWNLOAD
        try {
            URL url = new URL( urlString );
            URLConnection con = url.openConnection();
            con.connect();
            InputStream in = con.getInputStream();
            byte[] data = new byte[ in.available() ];

            while( in.read(data) != -1 ){
                result += new String(data);
            }

        } catch (MalformedURLException e) {
            // TODO Auto-generated catch block
            e.printStackTrace();
        } catch (IOException e) {
            // TODO Auto-generated catch block
            e.printStackTrace();
        }
        //SAVE
        saveFile( result );
    }

    private void saveFile( String r )
    {
        try {
            File f = new File( context.getFilesDir(), "myfile" );
            if( !f.exists() )
                f.createNewFile();

            FileOutputStream fos = new FileOutputStream(f);//context.openFileOutput(Filename, context.MODE_PRIVATE);
            fos.write( r.getBytes() );
            fos.close();

            Log.w( "PageDownloader", "got it!" );
        } catch (FileNotFoundException e) {
            e.printStackTrace();
        } catch (IOException e) {
            Log.w("PageDownloader", e.getMessage());
        }

    }

    public class testClass extends AsyncTask<Void, Integer, Void>
    {

        @Override
        protected Void doInBackground(Void... params) {
            _download();
            return null;
        }

    }
}

I call it from my onCreate method in my MainActivity.java :

PageDownloader p = new PageDownloader(this);
String url = "www.google.com/";
p.download( url );

How is this class written? Anything wrong or to improve?

I want to use this code to download several (maybe 5 up to 20) pages when I click on something. After I downloaded the source I want to extract information out of it. Maybe there is a way to make it faster.

Answer 1

Don't leak Context

It's not good to leak Context out of an Activity/Service. Your class needs just the output of context.getFilesDir(), so store just that:

private final File parentDir;

public PageDownloader(Context context) {
    parentDir = context.getFilesDir();
}

Hardcoded strings

It would be better if the filename was a parameter, not a hardcoded string. If you must hardcode, then at least make it a constant:

private static final String FILENAME = "tmpPage.html";

Hide internal methods

It's good to make everything private by default, and change to less strict access only when there is a good reason to do so. For example the _download method will only be called from your inner class, so there's no need to make it public, so change it to private.

Reading from a stream

The way you read from the input stream has many problems:

• It's inefficient to build long strings by appending to a String with +=. Use a StringBuilder instead.
• Since you initialized String result = null, as you append the input to it your end result will start with the literal string "null". You probably meant to initialize with String result = "".
• The available method of InputStream is not very useful in practice, and it can be misleading. I would recommend using a constant instead.
• You are not closing the input stream after reading from it, which can lead to a resource leak.
• You should not use e.printStackTrace(), but log instead
• The comments //DOWNLOAD and //SAVE are completely pointless

Variable and method naming violations

You're not following common naming practices: variable and method names should be camelCase and not start with _. Class names should be CamelCase (starting with capital letter). For example:

• download instead of _download
• TestClass instead of testClass

Of course, an even bigger issue with your names is that they are not descriptive. For example the variable r, the class TestClass, and many others. You should review ALL your variables/methods/classes and give them more descriptive names so your code reads like a story.

Suggested implementation

private static final String TAG = "PageDownloader";
private static final int BUFSIZE = 1024;

private void download(String urlString) {
    StringBuilder builder = new StringBuilder();
    InputStream in = null;
    try {
        URL url = new URL(urlString);
        URLConnection con = url.openConnection();
        con.connect();
        in = con.getInputStream();
        byte[] data = new byte[BUFSIZE];
        while (in.read(data) != -1) {
            builder.append(new String(data));
        }
    } catch (MalformedURLException e) {
        Log.e(TAG, e.getMessage(), e);
        return;
    } catch (IOException e) {
        Log.e(TAG, e.getMessage(), e);
        return;
    } finally {
        if (in != null) {
            try {
                in.close();
            } catch (IOException e) {
                Log.e(TAG, e.getMessage(), e);
            }
        }
    }
    saveFile(builder.toString());
}