Retrieving multiple versions of source code for a file

Question

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());

    }

Someone should of pointed out that ClientProtocolException is an IOException making catching ClientProtocolException pointless in this code. — Andrew T Finnell, Apr 9, 2012 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, 2012 at 15:31

luketorjussen · Accepted Answer · 2012-01-18 09:39:38Z

18

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

answered Jan 18, 2012 at 9:39

luketorjussen

4212 silver badges8 bronze badges

3

\$\begingroup\$ That's rather nifty. C# needs this. A lot. \$\endgroup\$
– Jesse C. Slicer
Jan 23, 2012 at 17:49

Add a comment |

palacsint · Accepted Answer · 2014-03-01 13:03:50Z

12

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:

You could extract out a

RestServices restServices = RestServices.getInstance();

variable.

You should create constants from repeating Strings, like "not available", "no before source code available" etc.
I'd extract out getFileName method for the first try-catch block, a getSourceCode for the second, and so on.

edited Mar 1, 2014 at 13:03

answered Jan 18, 2012 at 10:50

palacsint

29.9k9 gold badges79 silver badges156 bronze badges

2

\$\begingroup\$ Agreed - What's the point of catching an exception if you don't deal with it? \$\endgroup\$
– Clockwork-Muse
Jan 18, 2012 at 18:42

Add a comment |

palacsint · Accepted Answer · 2012-02-01 22:11:41Z

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.

You've declared getClassName and sendGetRequestJsonTextToString as void, but then return values from them. — Matt Ball, Apr 9, 2012 at 16:04

user10103user10103 · Accepted Answer · 2012-01-19 04:12:27Z

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());
}

But keep in mind that this will also catch unchecked RuntimeExceptions. — brabec, Jan 31, 2012 at 11:35

orip · Accepted Answer · 2012-06-21 18:32:20Z

1

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";
}

answered Jun 21, 2012 at 18:32

orip

1414 bronze badges

\$\begingroup\$ Note that you'll lose the stack trace. \$\endgroup\$
– luiscubal
Jun 22, 2012 at 14:06

Add a comment |

David DeMar · Accepted Answer · 2014-03-01 14:18:43Z

0

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;
    }
}

answered Mar 1, 2014 at 14:18

David DeMar

101

\$\begingroup\$ 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. \$\endgroup\$
– Marc-Andre
Mar 1, 2014 at 15:35
\$\begingroup\$ 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. \$\endgroup\$
– David DeMar
Mar 1, 2014 at 16:31

Add a comment |

Stack Exchange Network

Retrieving multiple versions of source code for a file

6 Answers 6

Your Answer

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

Hot Network Questions

Retrieving multiple versions of source code for a file

6 Answers 6

Your Answer

Sign up or log in

Post as a guest

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

Related

Hot Network Questions

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