5
\$\begingroup\$

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.

\$\endgroup\$
1
  • 1
    \$\begingroup\$ Welcome to Code Review! If you use a file manager app such as Total Commander for Android, you can at least access the external storage of the device. However, I suspect that your file is stored to the internal storage. Either way, I know a couple of things to say about your code... I might review it soon. \$\endgroup\$ May 28, 2014 at 16:25

1 Answer 1

8
\$\begingroup\$

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());
}
\$\endgroup\$
1
  • 2
    \$\begingroup\$ Very good answer, I'd just like to add that at runtime, it could be helpful for the user to see an AlertDialog or Toast when something goes wrong. Few users have adb logcat running when using Android Applications :) \$\endgroup\$ May 30, 2014 at 16:43

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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