patternjavaModerate
Webpage downloader
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
I'd simply name it
These could inside the download method:
Reference: Effective Java, 2nd edition, Item 45: Minimize the scope of local variables
-
The
-
Commons IO has a method which does the copying:
-
I'd remove the
-
I'd move the
-
Close the streams in a
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.