Exception handling, et al - How do I make this not “poor”?

Question

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:

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

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

3. 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 {
    private final String urlToPage;
    private final String saveToFileName;

    /**
     * @param urlToPage
     * @param saveToFilename
     */
    public JGet(String urlToPage, String saveToFilename) {
        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");
        }
        this.saveToFileName = saveToFilename;
        this.urlToPage = urlToPage;
        try {
            getContents();
        } catch (IOException e) {
            throw new IllegalArgumentException("The ability to save to the destination file must not be blocked");
        } catch (IllegalArgumentException e) {
            throw new IllegalArgumentException("The URL must not be malformed");
        }
    }

    /**
     * @throws IOException
     * @throws IllegalArgumentException
     */
    public void getContents() throws IOException, IllegalArgumentException {
        HttpClient httpClient = new DefaultHttpClient();
        HttpGet httpGet = new HttpGet(this.urlToPage);
        HttpResponse response = httpClient.execute(httpGet);
        HttpEntity entity = response.getEntity();
        IOUtils.copy(entity.getContent(), new FileOutputStream(this.saveToFileName));
    }

    /**
     * @param args
     */
    public static void main(String[] args) {
        new JGet(null, "bing.html");
        new JGet("http://www.bing.com/", null);
        new JGet("", "bing.html");
        new JGet("http://www.bing.com/", "");
        new JGet("http://www.bing.com/", "readonly.html");
        new JGet("Malformed URL", "bing.html");
        new JGet("http://www.bing.com/", "bing.html");
        new JGet("http://www.aw20.co.uk/a/img/logo.png", "logo.png");
    }

}

Answer 1

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:

1. 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");
   

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

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

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

5. 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?

Answer 2

There are a few things I see that I would want to refactor if I were working on this

1. The getContents() method throws exceptions, which are simply caught and re-thrown in the JGet constructor in a way that modifies the error message, but doesn't include the cause (the original exception). This can make troubleshooting the code more difficult because you're obscuring information about where the problem originated. When throwing a new exception you can pass in the exception that caused it as an extra parameter.
2. It's questionable whether or not the catching and re-throwing the exception(s) from getContents() is actually necessary. It might be helpful if the modified error message helps the programmer know the context of why it's an Illegal argument, but maybe not. In this case you're catching an IOException and re-throwing it as an IllegalArugumentException, while assuming that the only thing that could cause the IOException in the first place is that the destination file is not available. This is the kind of thing I'm talking about when I say that you're obscuring the root cause. Catching the IllegalArgumentException and then re-throwing it immediately with the, "The URL must not be malformed" message also possibly obscures something and in this case and appears completely redundant since you're not changing the type of exception or adding any useful information. Just let the exception roll up the chain as it naturally would.
3. The first half (or so) of the JGet constructor is functionality that should be wrapped in its own method for readability reasons (you might later alter what defines an "illegal" argument, but you'll want to keep that definition contained within a method that does that job of determining if it's illegal).

So, I would refactor this as something like:

/** Does what a JGet does (this is a joke comment)
  * 
  * @param uriToPage The URL to the page you want
  * @param saveToFilename The full or relative path to the file you want to save to
  *
  * @throws IllegalArgumentException if the either the urlToPage or saveToFilename
  *         parameters are null or blank.
  * @throws IOException if an I/O problem occurs either retrieving the results from the URL or saving the results to the file.
  */
public JGet(String urlToPage, String saveToFilename) throws IllegalAgumentException, IOException {
  this.saveToFileName = saveToFilename;
  this.urlToPage = urlToPage;

  validate();
  getContents();
}

/** Some useful documentation...
  */
private void validate() throws IllegalArgumentException {
  // your code for checking for invalid arguments
}


...the rest of it...

I think the restrictions are pretty ridiculous as well, but you could re-use an OS tool, assuming it was available and call out to curl -L [URL] or something like that.

Answer 3

Here are a few notes which were not mentioned earlier. Firstly, some about the specification:

1. The jGet package name does not follow the usual conventions:

   • Code Conventions for the Java Programming Language, 9 - Naming Conventions
   • Effective Java, 2nd edition, Item 56: Adhere to generally accepted naming conventions

2. The extends Object is unnecessary here:

   public JGet extends Object
   

   It's the default.

3. JGet is not a good class name. From Clean Code, page 25:

   Classes and objects should have noun or noun phrase names like Customer, WikiPage, Account, and AddressParser. [...] A class name should not be a verb.

Others already mentioned the questionable Object return type of the getContents method. They might expect that candidates start arguing about the specification although their answers does not suggest this.

Some notes about the code:

1. Comments like this are unnecessary:

   /**
    * @param urlToPage
    * @param saveToFilename
    */
   

   It says nothing more than the code already does, it's rather noise. (Clean Code by Robert C. Martin: Chapter 4: Comments, Noise Comments)

2. I'd use the existing libraries for input checks.

   checkArgument(StringUtils.isNotBlank(urlToPage), "urlToPage cannot be blank");
   checkArgument(StringUtils.isNotBlank(saveToFilename), "saveToFilename cannot be blank");
   

   It uses Preconditions from Guava and StringUtils from Apache Commons Lang.

   See also: Effective Java, 2nd edition, Item 47: Know and use the libraries

3. I don't agree with that that you should never throw a NullPointerException. If the constructor gets a null reference it's usually an error in the client code. NPEs are fine for these situations. (See also: Effective Java, 2nd edition, Item 38: Check parameters for validity and Item 60: Favor the use of standard exceptions)

4. A note about the length() check:

   if (urlToPage.length() == 0) { ... }
   

   The following is the same and easier to read:

   if (urlToPage.isEmpty()) { ... }
   

5. Calling overridable methods (like getContents) from constructors usually is not a good practice. See What's wrong with overridable method calls in constructors? and Effective Java 2nd Edition, Item 17: Design and document for inheritance, or else prohibit it

6. Please note that HttpEntity has a writeTo method. You could use this instead of IOUtils.copy. Anyway, if you don't use writeTo you should close the InputStream which was returned from entity.getContent() (as the EntityUtils.toString() method does, for example).

   InputStream contentStream = null;
   FileOutputStream fileOutputStream = null;
   try {
       contentStream = entity.getContent();
       fileOutputStream = new FileOutputStream(saveToFileName);
       IOUtils.copy(contentStream, fileOutputStream);
   } finally {
       IOUtils.closeQuietly(contentStream);
       IOUtils.closeQuietly(fileOutputStream);
   }
   

   It would be simpler with with the writeTo method:

   final FileOutputStream fileOutputStream = new FileOutputStream(saveToFileName);
   try {
       entity.writeTo(fileOutputStream);
   } finally {
       fileOutputStream.close();
   }
   

7. In the getContents you could use urlToPage instead of this.urlToPage and saveToFileName instead of this.saveToFileName. Modern IDEs use highlighting to separate local variables from instance variables.