Increase download speed of code

I am making a download manager and in Java. My code is currently working and downloads at 430-450 kb/s. The same file, when downloaded using downthemall, gives 440-460 speed, and when using an Internet download manager, it gives 500+ kb/s.

I am looking for performance pitfalls (inefficient use of CPU, memory and network speed) I have in my code.

```
import java.io.File;
import java.io.FileNotFoundException;
import java.io.IOException;
import java.io.RandomAccessFile;
import java.net.URI;
import java.nio.channels.Channels;
import java.nio.channels.FileChannel;
import java.nio.channels.ReadableByteChannel;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
import java.util.concurrent.atomic.AtomicInteger;
import java.util.concurrent.atomic.AtomicLong;
import java.util.concurrent.atomic.AtomicLongArray;
import org.apache.http.client.methods.CloseableHttpResponse;
import org.apache.http.client.methods.HttpGet;
import org.apache.http.impl.client.CloseableHttpClient;

public class Downloader {

// States:
// 1 = downloading
// 2 = completed
// 0 = paused
// -1 = failed
AtomicInteger currentState = new AtomicInteger(1);
String downloadDirectory;
URI uri;
int segments;
String fileName;
File fileFile;
RandomAccessFile file;
CloseableHttpClient client;
FileChannel fileChannel;
AtomicLong bytesDone;
int speed;
int avgSpeed[] = {0, 0};
AtomicLongArray stateArray;
States states;
long sizeOfEachSegment;
long sizeofFile;
ExecutorService threadService;
long[] intialState;

// Constructor
public Downloader(String downloadDirectory, URI uri, CloseableHttpClient client,
long[] initialState, long byteDone, int segments, long sizeOfEachSegment) {
this.speed = 0;

this.downloadDirectory = downloadDirectory;
this.uri = uri;
this.client = client;
this.intialState = initialState;
this.bytesDone = n

JavaDoc, JavaDoc, JavaDoc.

// States:
// 1 = downloading
// 2 = completed
// 0 = paused
// -1 = failed
AtomicInteger currentState = new AtomicInteger(1);

This is the very reason why there exists Enum. What you're using here is a called a "magic integer" and is bad for many reasons, like readability, usability and a documentation nightmare.

If you have numerical values that have meaning, use an Enum!

public Downloader(String downloadDirectory, URI uri, CloseableHttpClient client,
        long[] initialState, long byteDone, int segments, long sizeOfEachSegment) {

Why does your constructor accept so many values? Why does the constructor accept a value that stands for already downloaded bytes? And this is why you need JavaDoc.

Also a constructor should not accept so many arguments, it's confusing. Add reasonable defaults and getters/setters to change these options.

} catch (FileNotFoundException ex) {
        System.out.println("failed to create file");
    }

Logging exceptions to stdout is bad for multiple reasons, depending on how the application is launched there might no be a stdout, or the client does not want the program to print to stdout. Also the message you provide is useless for debugging purposes. Use a logger to log a detailed message and the exception.

Why is logging to stdout bad by default? I'm working with the library of a big software house which does print messages to stdout, something like:


An exception occurred, object was null.
An exception occurred, object was null.
An exception occurred, object was null.
An exception occurred, object was null.

And it does this everytime it's used. It's noise, it's even worse, it's useless noise. I spent 10 or so minutes the first time until I realized that this was neither a fatal message (operation completed successful), nor that it was coming from my code.

public class Segment implements Runnable {

Does this class need to be public and non-final?

In Effective Java, Item 17, it is said that every class that is not explicitly designed to be subclassed should be final.

Now I'm not that strict, but I do believe that every "one shot" class, every class that only has a single, restricted and scoped usage should be final. Especially classes that could be inlined, but are extracted for readability purposes. Making these classes private final sends the clear message "this is only used here, once...maybe twice if I was lucky".

avgSpeed[0] += speed;
avgSpeed[1]++;

This is bad, this should be two variables.

Overall the design of this class is odd.

class Downloader
    Downloader(...)
    startDownload() // Should be called start() or begin().

    class Segment // Should be private final and called SegmentDownloader

This raises multiple thoughts:

• How is a client notified that the download is complete?

• How does this class handle multiple calls to startDownload()?

• How does it handle failures?

• Why are there so many similar named variables?

• Why am I allowed to set segments and sizeOfEachSegment separately?

• Why are all variables package private?

Well, I'll try to answer some of these questions I've raised.

There should be a way to notify clients of completion, the easiest way is to provide a property isFinished and/or allowing to register a listener for the completion event.

At the moment the class doesn't handle multiple calls to startDownload() at all, it just goes downhill from there. There should be state check at the beginning of startDownload that let's the method fail if either the download is in progress or (if wished) the download is complete.

Failures are never reported in a useful way. Printing something from stdout is only useful for debugging purposes the moment you write the code. Use an appropriate logger and throw exceptions when necessary. For example startDownload should test if the target file exists and the target location can be written to, and if that fails it should throw an exception.

You have a lot of variables there. Are you sure you need all of them in the class scope? Can you limit the scope of some of them?

The constructor takes too many arguments, and confusing arguments that is. You should strip most of them out and provide getters/setters instead, so that they can be changed after the initialization if wished, like this:

Downloader downloader = new Donwloader(fromThisAddress, targetLocation);
downloader.setSegmentCount(5);
downloader.start();

Also why am I allowed to set some sort of internal state of the Downloader, like the HTTP client and the initial state?

All variables are package private, that's bad. Always limit the scope of everything to smallest scope possible and only expand it if necessary. Local variables should always be private, unless they are explicitly need to be touched from outside or you plan on letting an extending class touch them. Inner classes should be `private f

StackExchange Code Review Q#60900, answer score: 10