6
\$\begingroup\$

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.join();
            } catch (InterruptedException e1) {
                e1.printStackTrace();
            }
        }
    }

}


class DownloadWatch implements Runnable {

    Thread pth;
    HashMap<String, Downloader> ths;
    ArrayList<String> urls;

    DownloadWatch(HashMap<String, Downloader> dmap) {
        ths = dmap;
        urls = new ArrayList<String>(dmap.keySet());
        pth = new Thread(this, String.format("downloadWatch_%s", dmap.toString()));
        pth.start();

    }

    public void run() {
        boolean anyactive = true;
        printHeader(ths);
        // monotonic interval clock
        long now = System.nanoTime();
        for(;;) {
            try {
                Thread.sleep(101);
            } catch (InterruptedException e) {
                return;
            }
            long t = System.nanoTime();
            if (t - now < 900000000L)
                continue;
            now = System.nanoTime() - (t-now) + 1000000000L;
            printCompleted(ths);
            boolean allfinished = true;
            for(Downloader d : ths.values()) {
                if(d.isAlive()) {
                    allfinished = false;
                    break;
                }
            }
            if(allfinished)
                return;
        }
    }

    private void printCompleted(HashMap<String, Downloader> ths) {
        System.out.printf(new SimpleDateFormat("HH:mm:ss ").format(new Date()));
        for(String url : urls) {
            Downloader d = ths.get(url);
            long contlen = d.getContlen();
            long copied = d.getCopied();
            if (contlen == -1) {
                System.out.printf("? ");
            }
            else
            {
                if(copied == -1) {
                    System.out.printf("E ");
                }
                else
                {
                    double r = new Long(copied).doubleValue() / new Long(contlen).doubleValue();
                    System.out.printf("%.0f%% ", r*100);
                }
            }
        }
        System.out.println();
    }

    private void printHeader(HashMap<String, Downloader> ths) {
        for(String url : urls) {
            System.out.printf("%s ", FilenameUtils.getName(ths.get(url).getFilepath()));
        }
        System.out.println();
    }

    public void join() throws InterruptedException {
        pth.join();
    }
}

public class ParallelWget {

    String[] urls = null;
    String dirname;

    ParallelWget(String dirname, String[] urllist) {
        this.urls = urllist;
        this.dirname = dirname;
    }

    private HashMap<String,String> getFilePaths(String dirname, String[] urllist) {
        int cnt = 1;
        HashMap<String, String> fpaths = new HashMap<String, String>();
        for (String url : urllist) {
            String name = FilenameUtils.getName(url);
            if(!name.equals("")) {
                String fpt = new File(dirname, name).getAbsolutePath();
                if(fpaths.values().contains(fpt)) {
                    fpt = String.format("%s_%d", fpt, cnt++);
                }
                fpaths.put(url, fpt);
            }
        }
        return fpaths;
    }

    public static void main(String[] argv) {
        String cwd = System.getProperty("user.dir");
        ParallelWget pw = new ParallelWget(cwd, argv);
        pw.downloadWatch();
    }

    private void downloadWatch() {
        HashMap<String, String> urlfiles = getFilePaths(dirname, this.urls);
        HashMap<String, Downloader> ths = makeThreads(urlfiles);
        DownloadWatch dw = new DownloadWatch(ths);
        for(Downloader th : ths.values()) {
            try {
                th.join();
            } catch (InterruptedException e) {
                e.printStackTrace();
            }
        }
        try {
            dw.join();
        } catch (InterruptedException e) {
            e.printStackTrace();
        }
        printFooter(urlfiles);
    }

    private void printFooter(HashMap<String, String> urlfiles) {
        for(String url : urlfiles.keySet()){
            System.out.printf("- ");
        }
        System.out.println();
    }

    private HashMap<String, Downloader> makeThreads(HashMap<String, String> urlfiles) {
        HashMap<String, Downloader> ths = new HashMap<String, Downloader>();
        for(String url : urlfiles.keySet()) {
            String p = urlfiles.get(url);
            Downloader d;
            try {
                d = new Downloader(url, p);
            } catch (IOException e) {
                System.out.printf("Problem with URL %s : %s. Skipping URL.\n", url, e.getMessage());
                continue;
            }
            ths.put(url, d);
        }
        return ths;
    }

}
\$\endgroup\$
6
\$\begingroup\$

Right, cool programming challenge.

It appears, by scanning your code, that it does logical things in a relatively well structured OO way. I have written code similar to this a few times in the past, and it seems somehow familiar.

On the other hand, when I mean 'in the past', I mean before the advent of the java.util.concurrent.* API. That API changed much of what you are doing in a way that is far more predictable, and neater.

There are some problems....

There is no synchronization on the getCopied() method. If there is an exception and the copied variable is set to -1, it is possible that the reporting thread will never see the -1 value, and will report the size of the output regarfless. The use of the copied variable needs to be synchronized.

Additionally, your code will be significantly different again if you were to use the Java8 streams/lambdas.

So, as a review, I would say that using raw threads and runnables, and using simple join() calls on threads is a real problem here. Rewriting it using Executors and Futures would be highly recommended. Personally I would be happy with an Executor/Future implementation, but a Java8 parallel stream implementation would be recommended by many now too. Because the work is long-running work, it would fit well with the concept of a parallelStream().

Using Concurrent API

I am not about to rewrite this code, but let me go through some of the things you should be using:

I strongly recommend that you read through the Concurrency tutorials to see how it is applied.

Java8

In Java8, a 'simple' function can be declared to process a file, and then the various files/urls can be streamed in parallel through that function, returning a success status.

Using internal concurrency you can update a progress thread too. Something like:

HashMap<String, String> urlfiles = getFilePaths(dirname, this.urls);
ConcurrentHashMap<String, AtomicLong> progress = new ConcurrentHashMap<>();
for (String file : urlfiles.keySet()) {
    progress.put(file, new AtomicInteger());
}

urlfiles.entrySet().parallelStream()
    .forEach(entry -> processUrlToFile(enry.getValue(), entry.getKey(), progress.get(file)));

printDone(progress);
\$\endgroup\$
2
  • \$\begingroup\$ +1 for "Using Concurrent API"... I have not put much thought into using this or Java 8, largely because I'm limited (for now) to using Java 7. However, that limitation may soon be lifted and so those 2 last sections you wrote might be very useful, thanks.. \$\endgroup\$ – LetMeSOThat4U Sep 15 '14 at 20:21
  • 1
    \$\begingroup\$ @JohnDoe java.util.concurrent was actually added in Java 1.5, so you could be using it right now. \$\endgroup\$ – Chris Hayes Sep 15 '14 at 20:56
7
\$\begingroup\$

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<String, Downloader> ths. Really?


HashMap<String, String> 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;
\$\endgroup\$
3
  • \$\begingroup\$ Well I grant you re naming and all... It's not very good Java apparently - it's a language I have to use, not the one I'd like to use (like Python, Go, Haskell..). Would declaring this.url instead be better? Re 900000000L and milliseconds: I used it because System.nanoTime() is the closest I can get to monotonic clock in Java apparently and that returns nanoseconds. Anyway, even though you're critical I'm upvoting your answer. \$\endgroup\$ – LetMeSOThat4U Sep 15 '14 at 20:11
  • \$\begingroup\$ Name it just url and use this.url = url in the constructor. Concerning nanos, yes, they're monotonic, millis mayn't be. Write private static final double WHATEVER_THIS_CONST_MEANS = 900_000_000L; and everybody sees that it's 0.9 seconds. I'm always very critical, sometimes even harsh, but I can see that learning programming is a long way. But the naming can be improved easily (especially from the current state) and it helps a lot. Choosing "perfect" names is another story... \$\endgroup\$ – maaartinus Sep 15 '14 at 21:25
  • \$\begingroup\$ @JohnDoe Actually, apart from the naming it's not bad at all. I was so critical 'cause I was mostly speaking about the naming. \$\endgroup\$ – maaartinus Sep 16 '14 at 15:05

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.