10
\$\begingroup\$

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);
    }
}
\$\endgroup\$
  • \$\begingroup\$ 1. Can you use automatic resource management? stackoverflow.com/questions/2943542/using-keyword-in-java 2. Pass the article name directly to the constructor. 3. Take the url from command line. 4. Keep it simple - e.g. every method could have been static. \$\endgroup\$ – Leonid May 2 '12 at 20:56
  • \$\begingroup\$ I think I will not be able to take the url directly from command line because actually I have a bunch of url's. :( This is just a small program, and I hope to grow this program into a giant which will be able to handle thousands of url's automatically! :) \$\endgroup\$ – JSong May 5 '12 at 8:23
15
\$\begingroup\$
  1. 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.

  2. 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

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

  4. 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)

  5. 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 {...}
    
  6. 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.

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

\$\endgroup\$

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.