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.

I have the following code that checks for three different properties. Unfortunately each call can throw up to four exceptions that I all have to catch. Can I some how make the code more readable by reducing the amount of try/catch?

public void retrieveSourceCode() {
        try {
            fileName = RestServices.getInstance().getClassName(getSourceCodeURI()); 
        } catch (ClientProtocolException e) {
            fileName = "not available";
            e.printStackTrace();
        } catch (IOException e) {
            fileName = "not available";
            e.printStackTrace();
        } catch (RestServicesException e) {
            fileName = "not available";
            e.printStackTrace();
        }

        if (!(fileName.endsWith(".jar") || fileName.endsWith(".svn-base"))) {
            try {
                sourceCode = RestServices.getInstance().sendGetRequestJsonTextToString(getSourceCodeURI());
            } catch (ClientProtocolException e) {
                sourceCode = "not available";
                e.printStackTrace();
            } catch (IOException e) {
                sourceCode = "not available";
                e.printStackTrace();
            } catch (RestServicesException e) {
                sourceCode = "not available";
                e.printStackTrace();
            }

            if (before == null) {
                beforeSourceCode = "no before source code available";
            } else {
                try {
                    beforeSourceCode = RestServices.getInstance().sendGetRequestJsonTextToString(getBeforeVersionURI());
                } catch (ClientProtocolException e) {
                    beforeSourceCode = "no before source code available";
                    e.printStackTrace();
                } catch (IOException e) {
                    beforeSourceCode = "no before source code available";
                    e.printStackTrace();
                } catch (RestServicesException e) {
                    beforeSourceCode = "no before source code available";
                    e.printStackTrace();
                }
            }

            if (after == null) {
                afterSourceCode = "no after source code available";
            } else {
                try {
                    afterSourceCode = RestServices.getInstance().sendGetRequestJsonTextToString(getAfterVersionURI());
                } catch (ClientProtocolException e) {
                    afterSourceCode = "no after source code available";
                    e.printStackTrace();
                } catch (IOException e) {
                    afterSourceCode = "no after source code available";
                    e.printStackTrace();
                } catch (RestServicesException e) {
                    afterSourceCode = "no after source code available";
                    e.printStackTrace();
                }
            }
        }

        getChangeSet().addAffectedFile(getFileName());

    }
share|improve this question
1  
Someone should of pointed out that ClientProtocolException is an IOException making catching ClientProtocolException pointless in this code. –  Andrew Finnell Apr 9 '12 at 13:44
    
You may not have control over it, but needing to check "before" and "after" for null is a little smelly to me. –  Ryan Miller Apr 9 '12 at 15:31
add comment

6 Answers 6

If you upgrade to Java 7, you can use the new Multi-catch syntax. You could change the first block to:

    try {
        fileName = RestServices.getInstance().getClassName(getSourceCodeURI()); 
    } catch (ClientProtocolException | IOException | RestServicesException e) {
        fileName = "not available";
        e.printStackTrace();
    }

Essentially if there are many exceptions you catch and handle in the same way, you can use multi-catch

share|improve this answer
3  
That's rather nifty. C# needs this. A lot. –  Jesse C. Slicer Jan 23 '12 at 17:49
add comment

These unhandled exceptions smell very bad. Storing error messages in the place of data also is not a good sign (like fileName = "not available").

Anyway, some notes:

  1. You could extract out a

    RestServices restServices = RestServices.getInstance();
    

    variable.

  2. You should create constants from repeating Strings, like "not available", "no before source code available" etc.

  3. I'd extract out getFileName method for the first try-catch block, a getSourceCode for the second, and so on.

share|improve this answer
2  
Agreed - What's the point of catching an exception if you don't deal with it? –  Clockwork-Muse Jan 18 '12 at 18:42
add comment

A try and catch block should be in their own method so as not to interrupt logic in another method.

private void getClassName(T URI, String error){
    T2 className;
    try{
        className = RestServices.getInstance().getClassName(URI);
    } catch (Exception e){
        className = error;
        e.printStackTrace();
    } finally{
        return className;
    }
}

and

private void sendGetRequestJsonTextToString(T URI, String error){
    T2 reply;
    try {
        reply = RestServices.getInstance().sendGetRequestJsonTextToString(URI));
    } catch(Exception e){
        reply = error;
        e.printStackTrace();
    } finally{
        return reply;
    }
}

This method now has no try-catch block interrupting the logic. Also, guard clause can be used in place of nested ifs.

public void retrieveSourceCode(){
    String error;
    error = "not available";
    filename = getClassName(getSourceCodeURI(), error);

    // this conditional can be extracted to a method, giving more readability
    // say, if(!isSourcecode(filename))
    if(fileName.endsWith(".jar") || !fileName.endsWith(".svn-base"))){
        return;
    }

    error = "not available";
    sourceCode = sendGetRequestJsonTextToString(getSourceCodeURI(), error);


    error = "no before source code available";
    if(before == null){
        beforeSourceCode = error;
    } else {
        beforeSourceCode = sendGetRequestJsonTextToString(getBeforeVersionURI(), error);
    }


    error = "no after source code available";
    if(after == null){
        afterSourceCode = error;
    } else {
        afterSourceCode = sendGetRequestJsonTextToString(getAfterVersionURI(), error);
    }


    getChangeSet().addAffectedFile(getFileName());
}

This still has some smells (as mentioned in another answer), but it does what the questions asking for: to simplify the try-catch block.

share|improve this answer
    
You've declared getClassName and sendGetRequestJsonTextToString as void, but then return values from them. –  Matt Ball Apr 9 '12 at 16:04
add comment

Considering that you do literally the exact same thing regardless of the type of exception, you can cut down the catch statements by catching type Exception instead of a specific subclass:

public void retrieveSourceCode() {
    try {
        fileName = RestServices.getInstance().getClassName(getSourceCodeURI());    
    } catch (Exception e) {
        fileName = "not available";
        e.printStackTrace();
    }

    if (!(fileName.endsWith(".jar") || fileName.endsWith(".svn-base"))) {
        try {
            sourceCode = RestServices.getInstance().sendGetRequestJsonTextToString(getSourceCodeURI());
        } catch (Exception e) {
            sourceCode = "not available";
            e.printStackTrace();
        } 

        if (before == null) {
            beforeSourceCode = "no before source code available";
        } else {
            try {
                beforeSourceCode = RestServices.getInstance().sendGetRequestJsonTextToString(getBeforeVersionURI());
            } catch (Exception e) {
                beforeSourceCode = "no before source code available";
                e.printStackTrace();
            } 
        }

        if (after == null) {
            afterSourceCode = "no after source code available";
        } else {
            try {
                afterSourceCode = RestServices.getInstance().sendGetRequestJsonTextToString(getAfterVersionURI());
            } catch (Exception e) {
                afterSourceCode = "no after source code available";
                e.printStackTrace();
            } 
        }
    }
    getChangeSet().addAffectedFile(getFileName());
}
share|improve this answer
2  
But keep in mind that this will also catch unchecked RuntimeExceptions. –  prasopes Jan 31 '12 at 11:35
add comment

Java 7 syntax wins.

For Java < 7, catching the Exception base class works but it's unfortunate to stop unrelated exceptions from propagating. Here's a workaround to consider:

String getFileName() {
  try {
    return RestServices.getInstance().getClassName(getSourceCodeURI());
  } catch (ClientProtocolException e) {
  } catch (IOException e) {
  } catch (RestServicesException) {
  }
  return "not available";
}
share|improve this answer
    
Note that you'll lose the stack trace. –  luiscubal Jun 22 '12 at 14:06
add comment

You could use an if statement inside your catch. It could get ugly if you're trying to catch a lot of different exception types but it should be fine for a handful. Sorry for any errors in my syntax. It's been a while since I've used Java.

try {
    fileName = RestServices.getInstance().getClassName(getSourceCodeURI());  
}catch (Exception e){
    if (e instanceof ClientProtocolException || e instanceof IOException || e instanceof RestServicesException) {
        fileName = "not available";
        e.printStackTrace();
    }else{
        throw e;
    }
}
share|improve this answer
    
Welcome to Code Review! I'm glad you post an answer and hope you'll stay around. I'm not a big fan of this approach, this should get the job done, but is a lot of "boiler" plate IMHO. –  Marc-Andre Mar 1 at 15:35
    
I mostly agree, hence why I'd only use it with a small number of exceptions. Otherwise multiple catch blocks is probably the way to go, although it looks like the latest Java version has solved this problem. –  David DeMar Mar 1 at 16:31
add comment

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.