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

Parallel "wget" in Java

Submitted by: @import:stackexchange-codereview··
0
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

Solution

Just a rant, someone else will do the real work, but for me it's hard to read.

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 simpler

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