Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

This method is used to check a URL and return the time it takes to check and the HTTP status code:

public static String urlCheck(String url) {
    try {
        URL urlToCheck = new URL(url);
        HttpURLConnection connection = (HttpURLConnection) urlToCheck.openConnection();
        connection.setRequestMethod("GET");
        Long start = System.currentTimeMillis();
        connection.connect();
        int urlStatus = connection.getResponseCode();
        Long stop = System.currentTimeMillis();
        Long urlCheckTime = stop - start;
        return (connection.getRequestMethod() + " " + urlToCheck + " "
                + urlStatus + " " + urlCheckTime + "ms");
    }
    catch (IOException err){
        logger.log(Level.WARNING,err.getMessage());
        return "Skipping URL";
    }

}

My questions are:

  1. Should my try block contain all the lines that are in it now, or should it only contain the line that can throw an exception (I put them all in since variables were not resolving (e.g. urlToCheck on first line was not passed to the second line if the second line was outside the try block))?
  2. Is using the blanket exception IOException ok, or would it be better to create a catch block for each exception type that the try block can throw?
share|improve this question

2 Answers 2

up vote 4 down vote accepted

Blanked Exception or specific Exceptions

It depends on what you want to do with the exceptions. If you just want to stop and return "not working", then your approach is good.

But if you want to try and rescue the attempt, then a different approach would be better, and then you have to differentiate by exception type:

public static String urlCheck(String url) {
    URL urlToCheck;
    try {
        urlToCheck = new URL(url);
    } catch (MalformedURLException ex) {
        // try to clean url (eg prepend http, etc)
        // or:
        return "url is not valid";
    }
    HttpURLConnection connection = null;
    try {
        connection = (HttpURLConnection) urlToCheck.openConnection();
        [... your connection code ...]
    } catch (IOException ex) {
        return "could not connect to server";
    } finally {
        if (connection != null) {
            connection.disconnect();
        }
    }
}

If you don't want to recover from malformed urls, you could simply use multiple catch clauses:

public static String urlCheck3(String url) {
    try {
        [... your connection code ...]
    } catch (MalformedURLException ex) {
        return "url is not valid";
    } catch (IOException ex) {
        return "could not connect to server";
    }
}

I think it's better to return more specific errors, but if you don't care about the error in this case, then the blanket IOException would be fine as well.

Content of try block

I would put everything that depends on the code inside the try block inside the try block as well. For example, having connection.getRequestMethod() outside the try block if a connection couldn't be established wouldn't make sense.

share|improve this answer

Generally, you should try and minimize the number of statements in a try block to the minimum possible.

However, in your case, since

URL urlToCheck = new URL(url);
HttpURLConnection connection = (HttpURLConnection) urlToCheck.openConnection();
connection.setRequestMethod("GET");

and

connection.connect();
int urlStatus = connection.getResponseCode();

can all throw exceptions, and the rest of the code is relatively minor (5 lines in your version or 4 in mine), I would leave it all as is, in the try block.

I would recommend eliminating stop and writing the timing code like this:

Long start = System.currentTimeMillis();
int urlStatus = connection.getResponseCode();
Long urlCheckTime = System.currentTimeMillis() - start;

This will yield slightly more accurate timing and eliminates a unneeded variable. You can also eliminate urlCheckTime by having the code in the return string but that's a matter of taste.

As for whether to use a blanket IOException or to have individual catch blocks for MalformedUrlException and so on, that is a question that you need to answer. If you want to have more detailed output, you will need the additional catch blocks. If however, you are simply testing to see if the URL is working, then your method is fine.

share|improve this answer
    
Excellent feedback. Thanks for helping to cut a line of code. –  moto_beats Aug 20 at 18:40

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.