Exception handling, et al - How do I make this web downloader not "poor"?

I haven't done Java coding in years, but I thought I would give it a shot for a job interview. I have a few questions:

-
Why is this error handling considered poor? How am I supposed to be doing it?

-
What error catching should I be doing in a finally clause?

-
How am I masking exceptions?

```
package jGet;

/**
* Problem
*
* Create a very simple Java class that will retrieve the resource of any URL (using the HTTP protocol)
* and save the contents as seen by the browser, to a file.
*
* Restrictions
*
* You are free to use any library/technique, except for java.net.Url, java.net.URI or java.net.UrlConnection.
* Solutions using these classes will not be accepted.
* You are free to change the class signature for better error handling and readability.
*
* Initial Class outline
*
* public JGet extends Object {
* public JGet( String urlToPage, String saveToFilename ) {
* }
* public Object getContents() {
* }
* }
*
* Sample Test cases
*
* The class should be able to download the following sample URL's to a file:
* http://www.bing.com/
* http://www.aw20.co.uk/images/logo.png
*
* Time Allowance
*
* This took me a long time to complete (longer than 30 minutes).
* I returned the completed .java file to the person who invited me to take this, and
* this is the feedback I got back:
* 1) Poor exception handling
* 2) No finally clause in case of errors
* 3) getContents() returns a void
* 4) You should never throw a NullPointerException
* 5) Exception masking
* 6) Constructor calls the function getContents() no matter what
* 7) Poor layout
*
*/

import java.io.FileOutputStream;
import java.io.IOException;

import org.apache.commons.io.IOUtils;
import org.apache.http.HttpEntity;
import org.apache.http.HttpResponse;
import org.apache.http.client.HttpClient;
import org.apache.http.client.methods.HttpGet;
import org.apache.http.impl.client.DefaultHttpClient;

public class JGet

Let me start off by saying how ridiculous the restrictions are:

You are free to use any library/technique, except for java.net.Url, java.net.URI
or java.net.UrlConnection.
Solutions using these classes will not be accepted.

The fact is that the apache libraries which you are using internally use java.net.Url and java.net.URI.

There are a few things that I'd like to point out about your code:

-
There is some redundancy that can be avoided in the argument validation:

if (urlToPage == null) {
    throw new IllegalArgumentException("The URL must not be null");
}
if (saveToFilename == null) {
    throw new IllegalArgumentException(
            "The name of the destination file must not be null");
}
if (urlToPage.length() == 0) {
    throw new IllegalArgumentException("The URL must not be blank");
}
if (saveToFilename.length() == 0) {
    throw new IllegalArgumentException(
            "The name of the destination file must not be blank");
}

Why don't we express this repeated logic in a validation method:

private static void validate(Object argument, Object illegal, String message){
    // reference == to avoid NullPointerException
    if (argument == illegal || argument.equals(illegal))
        throw new IllegalArgumentException(message);
}

This cleans up that part of the constructor considerably:

validate(urlToPage, null, "The URL must not be null");
validate(saveToFileName, null, "The name of the destination file must not be null");
validate(urlToPage.length(), 0, "The URL must not be blank");
validate(saveToFileName.length(), 0, "The destination file name must not be blank");

-
The criticism Constructor calls the function getContents() no matter what is justified in the sense that your constructor shouldn't do any complex work (such as file downloading) at all. So just set the two fields and be done. The file download should be explicitly started after creation of an instance.

-
The try-catch at the end of your constructor does hide IOException as IllegalArgumentException. Avoid ever throwing IllegalArgumentException if it isn't right at the start of your method, or you will confuse your callers. Remove all this as you shouldn't be doing the download in the constructor anyway.

-
You are right: Object is not an adequate return type for getContents. In this situation it would have been important to demand an explanation as to what kind of return is expected!

However: if you change the method to return void, you also need to change the name as get is no longer an accurate verb. I'd simply go with download.

-
This may be a bit subjective, but I wouldn't specify Exceptions in the throws clause unless they are checked exceptions and therefore have to be included. So I'd leave out the throws IllegalArgumentException. The finally criticism was presumably referring to the unclosed output stream:

public void download() throws IOException {
    HttpClient httpClient = new DefaultHttpClient();
    HttpResponse response = httpClient.execute(new HttpGet(urlToPage));
    HttpEntity entity = response.getEntity();
    FileOutputStream stream = null;
    try {
        stream = new FileOutputStream(this.saveToFileName);
        IOUtils.copy(entity.getContent(), stream);
    }
    finally{
        if (stream != null) 
            stream.close();
    }
}

All in all, I don't consider this exercise to be effective in showing if someone is a good programmer or not. The object-orientation is contrived; a static utility class would be a lot more suitable for task. Who wants to instantiate a new object for every download, especially when none of the features of object-orientation (inheritance, polymorphism, ...) are used beneficially?

StackExchange Code Review Q#15420, answer score: 11