HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaModerate

Webpage downloader

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
downloaderwebpagestackoverflow

Problem

I wanted to download a webpage. I confirmed that this code works. But I'm not sure whether I'm doing it neatly. Would you review my (full) code?

import java.io.IOException;
import java.io.InputStream;
import java.io.FileOutputStream;
import java.net.URL;

public class GetHTML {
    private InputStream in;
    private FileOutputStream out;
    private String articleName;

    public void setArticleName(String articleName) {
        this.articleName = articleName;
    }

    public void download(URL url) throws IOException {
        in = url.openStream();
        out = new FileOutputStream(articleName + ".html");
        byte[] arr = new byte[1024];
        while(true) {
            int count = in.read(arr);
            if(count == -1) { break; }
            out.write(arr, 0, count);
        }
        in.close();
        out.close();
    }

    public static void main(String[] args) throws IOException {
        String site1 = "http://codereview.stackexchange.com/questions/";
        String site2 = "69";
        String site3 = "is-this-implementation-of-shamos-hoey-algorithm-ok";
        URL url = new URL(site1 + site2 + "/" + site3);
        GetHTML getHTML = new GetHTML();

        getHTML.setArticleName("[" + site2 + "]" + site3);
        getHTML.download(url);
    }
}

Solution

-
From Clean Code, page 25:


Classes and objects should have noun or noun phrase names like Customer, WikiPage,
Account, and AddressParser. [...] A class name should not be a verb.

I'd simply name it ArticleDownloader, for example.

  • -



private InputStream in;
private FileOutputStream out;


These could inside the download method:

public void download(URL url) throws IOException {
    final InputStream in = url.openStream();
    final OutputStream out = new FileOutputStream(articleName + ".html");


Reference: Effective Java, 2nd edition, Item 45: Minimize the scope of local variables

-
The FileOutputStream out could be just OutputStream type. (Effective Java, 2nd edition, Item 52: Refer to objects by their interfaces)

-
Commons IO has a method which does the copying: IOUtils.copy(InputStream input, OutputStream output) throws IOException. (Effective Java, 2nd edition, Item 47: Know and use the libraries)

-
I'd remove the setArticleName method and the articleName field (since there is no other method which uses this field) and pass it directly to the download method:

public void download(URL url, final String articleName) throws IOException {...}


-
I'd move the main method to a separate class. (For example, DownloaderMain.) It's usually a good idea to separate a class from its clients.

-
Close the streams in a finally block. (Effective Java, 2nd edition, Item 7: Avoid finalizers)

Code Snippets

private InputStream in;
private FileOutputStream out;
public void download(URL url) throws IOException {
    final InputStream in = url.openStream();
    final OutputStream out = new FileOutputStream(articleName + ".html");
public void download(URL url, final String articleName) throws IOException {...}

Context

StackExchange Code Review Q#11418, answer score: 15

Revisions (0)

No revisions yet.