patternjavaMinor
Parallel "wget" in Java
Viewed 0 times
javaparallelwget
Problem
Purpose: write Java program that downloads a list of URLs specified on commandline in parallel (simultaneously), reporting download completion every second.
My solution follows below, please point out areas for improvement / things you would redesign.
```
package org.test.mk.pjwget;
import java.io.*;
import java.net.URL;
import java.net.URLConnection;
import java.text.SimpleDateFormat;
import java.util.ArrayList;
import java.util.Date;
import java.util.HashMap;
import org.apache.commons.io.FilenameUtils;
import org.apache.commons.io.IOUtils;
import org.apache.commons.io.output.CountingOutputStream;
class Downloader implements Runnable {
private Thread pth;
private String durl;
private long contlen;
private long copied = 0;
private InputStream inp;
public String getFilepath() {
return filepath;
}
private String filepath;
private CountingOutputStream cos;
public long getContlen() {
return contlen;
}
public boolean isAlive() {
return pth.isAlive();
}
public long getCopied() {
if(copied > -1)
{
copied = cos.getByteCount();
}
return copied;
}
public void join() throws InterruptedException {
pth.join();
}
Downloader (String url, String filepath) throws IOException {
durl = url;
get_http_response(url);
this.filepath = filepath;
cos = new CountingOutputStream(new FileOutputStream(filepath));
pth = new Thread(this, url);
pth.start();
}
private void get_http_response(String url) throws IOException {
URL u = new URL(url);
URLConnection conn = u.openConnection();
contlen = conn.getContentLengthLong();
inp = conn.getInputStream();
}
public void run() {
try {
IOUtils.copy(inp, cos);
cos.flush();
} catch (IOException e) {
copied = -1;
try {
pth.jo
My solution follows below, please point out areas for improvement / things you would redesign.
```
package org.test.mk.pjwget;
import java.io.*;
import java.net.URL;
import java.net.URLConnection;
import java.text.SimpleDateFormat;
import java.util.ArrayList;
import java.util.Date;
import java.util.HashMap;
import org.apache.commons.io.FilenameUtils;
import org.apache.commons.io.IOUtils;
import org.apache.commons.io.output.CountingOutputStream;
class Downloader implements Runnable {
private Thread pth;
private String durl;
private long contlen;
private long copied = 0;
private InputStream inp;
public String getFilepath() {
return filepath;
}
private String filepath;
private CountingOutputStream cos;
public long getContlen() {
return contlen;
}
public boolean isAlive() {
return pth.isAlive();
}
public long getCopied() {
if(copied > -1)
{
copied = cos.getByteCount();
}
return copied;
}
public void join() throws InterruptedException {
pth.join();
}
Downloader (String url, String filepath) throws IOException {
durl = url;
get_http_response(url);
this.filepath = filepath;
cos = new CountingOutputStream(new FileOutputStream(filepath));
pth = new Thread(this, url);
pth.start();
}
private void get_http_response(String url) throws IOException {
URL u = new URL(url);
URLConnection conn = u.openConnection();
contlen = conn.getContentLengthLong();
inp = conn.getInputStream();
}
public void run() {
try {
IOUtils.copy(inp, cos);
cos.flush();
} catch (IOException e) {
copied = -1;
try {
pth.jo
Solution
Just a rant, someone else will do the real work, but for me it's hard to read.
When I see it..... what to say?
So slowly... "url" is fine, everybody understands it. "durl" could be maybe "dural" with a typo. Or "dull". There's no reason not to use the same name for a member and constructor argument. The naming in general is just terrible: "Thread pth", "long contlen". Look at other CRs for ideas, it's all the same.
So what is it? "urlFiles" or "filePaths"? I know, it's hard to keep this consistent, but try harder. Rename one or the other.
as it evaluates from left and upcasts to double.
All variables should be
Declare constants instead of magic numbers in code. I'd use milliseconds instead of nanos as it all takes ages, but you have a good point: System.currentTimeMillis` is not guaranteed to be monotonic, so let's stick with nanos. Just declare a constant
durl = url;When I see it..... what to say?
So slowly... "url" is fine, everybody understands it. "durl" could be maybe "dural" with a typo. Or "dull". There's no reason not to use the same name for a member and constructor argument. The naming in general is just terrible: "Thread pth", "long contlen". Look at other CRs for ideas, it's all the same.
get_http_response should be getHttpResponse, at least in Java.HashMap ths. Really?HashMap urlfiles = getFilePaths(...)So what is it? "urlFiles" or "filePaths"? I know, it's hard to keep this consistent, but try harder. Rename one or the other.
new Long(copied).doubleValue() should be simply (double) copied. Even simplerSystem.out.printf("%.0f%% ", 100.0 * copied / contlen);as it evaluates from left and upcasts to double.
All variables should be
private (and they're) and final (if possible).if (t - now < 900000000L)Declare constants instead of magic numbers in code. I'd use milliseconds instead of nanos as it all takes ages, but you have a good point: System.currentTimeMillis` is not guaranteed to be monotonic, so let's stick with nanos. Just declare a constant
private static final double WHATEVER_THIS_CONST_MEANS = 900_000_000L;Code Snippets
durl = url;HashMap<String, String> urlfiles = getFilePaths(...)System.out.printf("%.0f%% ", 100.0 * copied / contlen);if (t - now < 900000000L)private static final double WHATEVER_THIS_CONST_MEANS = 900_000_000L;Context
StackExchange Code Review Q#62967, answer score: 7
Revisions (0)
No revisions yet.