You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@ofbiz.apache.org by Adrian Crum <ad...@sandglass-software.com> on 2012/05/06 11:13:28 UTC
Try/Catch Patterns (was: svn commit: r1334497 ...)
On 5/6/2012 10:07 AM, Jacques Le Roux wrote:
> Nobody is speaking about swallowing, I hope we have not such things in
> OFBiz! (though Scott's wait to read the end before thinking I did not
> get you and being upset ;o)
>
> No, it's more about stuff like
>
> } catch (BirtException e) {
> throw new ViewHandlerException("Birt Error create engine: " +
> e.toString(), e);
> } catch (IOException e) {
> throw new ViewHandlerException("Error in the response writer/output
> stream: " + e.toString(), e);
> } catch (SQLException e) {
> throw new ViewHandlerException("get connection error: " +
> e.toString(), e);
> } catch (GenericEntityException e) {
> throw new ViewHandlerException("generic entity error: " +
> e.toString(), e);
> } catch (GeneralException e) {
> throw new ViewHandlerException("general error: " + e.toString(), e);
> } catch (SAXException se) {
> String errMsg = "Error SAX rendering " + page + " view handler: " +
> se.toString();
> Debug.logError(se, errMsg, module);
> throw new ViewHandlerException(errMsg, se);
> } catch (ParserConfigurationException pe) {
> String errMsg = "Error parser rendering " + page + " view handler:
> " + pe.toString();
> Debug.logError(pe, errMsg, module);
> throw new ViewHandlerException(errMsg, pe);
> }
>
>
> } catch (GeneralException ge) {
> String errMsg = "Error rendering " + birtContentType + " attachment
> for email: " + ge.toString();
> Debug.logError(ge, errMsg, module);
> return ServiceUtil.returnError(errMsg);
> } catch (IOException ie) {
> String errMsg = "Error I/O rendering " + birtContentType + "
> attachment for email: " + ie.toString();
> Debug.logError(ie, errMsg, module);
> return ServiceUtil.returnError(errMsg);
> } catch (FOPException fe) {
> String errMsg = "Error FOP rendering " + birtContentType + "
> attachment for email: " + fe.toString();
> Debug.logError(fe, errMsg, module);
> return ServiceUtil.returnError(errMsg);
> } catch (SAXException se) {
> String errMsg = "Error SAX rendering " + birtContentType + "
> attachment for email: " + se.toString();
> Debug.logError(se, errMsg, module);
> return ServiceUtil.returnError(errMsg);
> } catch (ParserConfigurationException pe) {
> String errMsg = "Error parser rendering " + birtContentType + "
> attachment for email: " + pe.toString();
> Debug.logError(pe, errMsg, module);
> return ServiceUtil.returnError(errMsg);
> } catch (EngineException ee) {
> String errMsg = "Error rendering " + birtContentType + " attachment
> for email: " + ee.toString();
> Debug.logError(ee, errMsg, module);
> return ServiceUtil.returnError(errMsg);
> } catch (SQLException se) {
> String errMsg = "Error SQL rendering " + birtContentType + "
> attachment for email: " + se.toString();
> Debug.logError(se, errMsg, module);
> return ServiceUtil.returnError(errMsg);
> }
>
> I think the
> exception.toString()
> Debug.logError(exception, errMsg, module);
> information is enough. This for 4 reasons:
>
> 1. exception.toString(), and exception in Debug.logError give enough
> information. No needs to specify things like
> "Error I/O rendering "
> "Error FOP rendering"
> etc.
> this will anyway be given by the thrown exception
> 2. It's more legible
> 3. It's an antipattern people C/P (like I did) so it spreads
> everywhere in code
> 4. Those kinds of exception are exceptionally thrown. BTW I personaly
> believe checked exceptions in Java is not really the best part
> of the language. As soons as it has spread in library and at large
> code, you have to suffer all those try/catch blocks
>
> So this like below, this can be rewritten
>
> } catch (Exception e) {
> String errMsg = "Error rendering " + birtContentType + " attachment
> for email: " + e.toString();
> Debug.logError(e, errMsg, module);
> return ServiceUtil.returnError(errMsg);
> }
>
The "+ e.toString()" is not needed because Debug will print the
Exception's message anyway. I usually delete those because they make the
log harder to read.
-Adrian
Re: Try/Catch Patterns (was: svn commit: r1334497 ...)
Posted by Jacques Le Roux <ja...@les7arts.com>.
From: "Scott Gray" <sc...@hotwaxmedia.com>
> On 6/05/2012, at 10:43 PM, Jacques Le Roux wrote:
>
>> From: "Scott Gray" <sc...@hotwaxmedia.com>
>>> On 6/05/2012, at 9:28 PM, Jacques Le Roux wrote:
>>>
>>>> From: "Adrian Crum" <ad...@sandglass-software.com>
>>>>> On 5/6/2012 10:07 AM, Jacques Le Roux wrote:
>>>>>> Nobody is speaking about swallowing, I hope we have not such things in OFBiz! (though Scott's wait to read the end before
>>>>>> thinking I did not get you and being upset ;o)
>>>>>>
>>>>>> No, it's more about stuff like
>>>>>>
>>>>>> } catch (BirtException e) {
>>>>>> throw new ViewHandlerException("Birt Error create engine: " + e.toString(), e);
>>>>>> } catch (IOException e) {
>>>>>> throw new ViewHandlerException("Error in the response writer/output stream: " + e.toString(), e);
>>>>>> } catch (SQLException e) {
>>>>>> throw new ViewHandlerException("get connection error: " + e.toString(), e);
>>>>>> } catch (GenericEntityException e) {
>>>>>> throw new ViewHandlerException("generic entity error: " + e.toString(), e);
>>>>>> } catch (GeneralException e) {
>>>>>> throw new ViewHandlerException("general error: " + e.toString(), e);
>>>>>> } catch (SAXException se) {
>>>>>> String errMsg = "Error SAX rendering " + page + " view handler: " + se.toString();
>>>>>> Debug.logError(se, errMsg, module);
>>>>>> throw new ViewHandlerException(errMsg, se);
>>>>>> } catch (ParserConfigurationException pe) {
>>>>>> String errMsg = "Error parser rendering " + page + " view handler: " + pe.toString();
>>>>>> Debug.logError(pe, errMsg, module);
>>>>>> throw new ViewHandlerException(errMsg, pe);
>>>>>> }
>>>>>>
>>>>>>
>>>>>> } catch (GeneralException ge) {
>>>>>> String errMsg = "Error rendering " + birtContentType + " attachment for email: " + ge.toString();
>>>>>> Debug.logError(ge, errMsg, module);
>>>>>> return ServiceUtil.returnError(errMsg);
>>>>>> } catch (IOException ie) {
>>>>>> String errMsg = "Error I/O rendering " + birtContentType + " attachment for email: " + ie.toString();
>>>>>> Debug.logError(ie, errMsg, module);
>>>>>> return ServiceUtil.returnError(errMsg);
>>>>>> } catch (FOPException fe) {
>>>>>> String errMsg = "Error FOP rendering " + birtContentType + " attachment for email: " + fe.toString();
>>>>>> Debug.logError(fe, errMsg, module);
>>>>>> return ServiceUtil.returnError(errMsg);
>>>>>> } catch (SAXException se) {
>>>>>> String errMsg = "Error SAX rendering " + birtContentType + " attachment for email: " + se.toString();
>>>>>> Debug.logError(se, errMsg, module);
>>>>>> return ServiceUtil.returnError(errMsg);
>>>>>> } catch (ParserConfigurationException pe) {
>>>>>> String errMsg = "Error parser rendering " + birtContentType + " attachment for email: " + pe.toString();
>>>>>> Debug.logError(pe, errMsg, module);
>>>>>> return ServiceUtil.returnError(errMsg);
>>>>>> } catch (EngineException ee) {
>>>>>> String errMsg = "Error rendering " + birtContentType + " attachment for email: " + ee.toString();
>>>>>> Debug.logError(ee, errMsg, module);
>>>>>> return ServiceUtil.returnError(errMsg);
>>>>>> } catch (SQLException se) {
>>>>>> String errMsg = "Error SQL rendering " + birtContentType + " attachment for email: " + se.toString();
>>>>>> Debug.logError(se, errMsg, module);
>>>>>> return ServiceUtil.returnError(errMsg);
>>>>>> }
>>>>>>
>>>>>> I think the
>>>>>> exception.toString()
>>>>>> Debug.logError(exception, errMsg, module);
>>>>>> information is enough. This for 4 reasons:
>>>>>>
>>>>>> 1. exception.toString(), and exception in Debug.logError give enough information. No needs to specify things like
>>>>>> "Error I/O rendering "
>>>>>> "Error FOP rendering"
>>>>>> etc.
>>>>>> this will anyway be given by the thrown exception
>>>>>> 2. It's more legible
>>>>>> 3. It's an antipattern people C/P (like I did) so it spreads everywhere in code
>>>>>> 4. Those kinds of exception are exceptionally thrown. BTW I personaly believe checked exceptions in Java is not really the
>>>>>> best part
>>>>>> of the language. As soons as it has spread in library and at large code, you have to suffer all those try/catch blocks
>>>>>>
>>>>>> So this like below, this can be rewritten
>>>>>>
>>>>>> } catch (Exception e) {
>>>>>> String errMsg = "Error rendering " + birtContentType + " attachment for email: " + e.toString();
>>>>>> Debug.logError(e, errMsg, module);
>>>>>> return ServiceUtil.returnError(errMsg);
>>>>>> }
>>>>>>
>>>>> The "+ e.toString()" is not needed because Debug will print the Exception's message anyway. I usually delete those because
>>>>> they make the log harder to read.
>>>>> -Adrian
>>>>
>>>> Yes indeed, when you use the ternary version of Debug.log like in "Debug.logError(e, errMsg, module);"
>>>> Another thing we can do to clean the error stack in log...
>>>>
>>>> Jacques
>>>
>>>
>>> 1. There's nothing wrong with swallowing exceptions if the situation calls for it
>>> 2. I was merely pointing out that you can't simply take Adrian's comment and apply it everywhere as some golden rule. Not only
>>> that, most resources I've seen consider it to be an anti-pattern anyway.
>>>
>>> Regards
>>> Scott
>>
>> About 2. : I consider it an anti-pattern because, in cases like above, it clutters the code uselessly to give redundant
>> information (also given by the exception itself).
>> I did not mean an anti-pattern in all situations of course, especially when you want to give more information than only what the
>> exception can tell.
>>
>> So if nobody disagree, in case like above, I will removed "useless" exceptions catches in current code.
>>
>> Jacques
>
> I disagree, I was referring to Adrian's suggestion as an anti-pattern. The whole reason I replied is because I knew someone
> reading his comment would take it upon themselves to begin mass converting the entire codebase, as if some gospel had just been
> handed down that must be spread throughout the land. One reason not to do it is because you will now be catching all exceptions
> even the ones you don't know about that may be added to the API in the future and may require different handling. Just because
> everything is handled in the same manner now doesn't mean that it will be in the future. If we follow this path we won't even
> know that a new type exception has been added.
>
> The way things are now does absolutely no harm and is safer than what you are proposing, at the mere cost of a few extra lines of
> code I don't see a need to change them.
>
> Regards
> Scott
OK, no worries
Jacques
Re: Try/Catch Patterns (was: svn commit: r1334497 ...)
Posted by Scott Gray <sc...@hotwaxmedia.com>.
On 6/05/2012, at 10:43 PM, Jacques Le Roux wrote:
> From: "Scott Gray" <sc...@hotwaxmedia.com>
>> On 6/05/2012, at 9:28 PM, Jacques Le Roux wrote:
>>
>>> From: "Adrian Crum" <ad...@sandglass-software.com>
>>>> On 5/6/2012 10:07 AM, Jacques Le Roux wrote:
>>>>> Nobody is speaking about swallowing, I hope we have not such things in OFBiz! (though Scott's wait to read the end before thinking I did not get you and being upset ;o)
>>>>>
>>>>> No, it's more about stuff like
>>>>>
>>>>> } catch (BirtException e) {
>>>>> throw new ViewHandlerException("Birt Error create engine: " + e.toString(), e);
>>>>> } catch (IOException e) {
>>>>> throw new ViewHandlerException("Error in the response writer/output stream: " + e.toString(), e);
>>>>> } catch (SQLException e) {
>>>>> throw new ViewHandlerException("get connection error: " + e.toString(), e);
>>>>> } catch (GenericEntityException e) {
>>>>> throw new ViewHandlerException("generic entity error: " + e.toString(), e);
>>>>> } catch (GeneralException e) {
>>>>> throw new ViewHandlerException("general error: " + e.toString(), e);
>>>>> } catch (SAXException se) {
>>>>> String errMsg = "Error SAX rendering " + page + " view handler: " + se.toString();
>>>>> Debug.logError(se, errMsg, module);
>>>>> throw new ViewHandlerException(errMsg, se);
>>>>> } catch (ParserConfigurationException pe) {
>>>>> String errMsg = "Error parser rendering " + page + " view handler: " + pe.toString();
>>>>> Debug.logError(pe, errMsg, module);
>>>>> throw new ViewHandlerException(errMsg, pe);
>>>>> }
>>>>>
>>>>>
>>>>> } catch (GeneralException ge) {
>>>>> String errMsg = "Error rendering " + birtContentType + " attachment for email: " + ge.toString();
>>>>> Debug.logError(ge, errMsg, module);
>>>>> return ServiceUtil.returnError(errMsg);
>>>>> } catch (IOException ie) {
>>>>> String errMsg = "Error I/O rendering " + birtContentType + " attachment for email: " + ie.toString();
>>>>> Debug.logError(ie, errMsg, module);
>>>>> return ServiceUtil.returnError(errMsg);
>>>>> } catch (FOPException fe) {
>>>>> String errMsg = "Error FOP rendering " + birtContentType + " attachment for email: " + fe.toString();
>>>>> Debug.logError(fe, errMsg, module);
>>>>> return ServiceUtil.returnError(errMsg);
>>>>> } catch (SAXException se) {
>>>>> String errMsg = "Error SAX rendering " + birtContentType + " attachment for email: " + se.toString();
>>>>> Debug.logError(se, errMsg, module);
>>>>> return ServiceUtil.returnError(errMsg);
>>>>> } catch (ParserConfigurationException pe) {
>>>>> String errMsg = "Error parser rendering " + birtContentType + " attachment for email: " + pe.toString();
>>>>> Debug.logError(pe, errMsg, module);
>>>>> return ServiceUtil.returnError(errMsg);
>>>>> } catch (EngineException ee) {
>>>>> String errMsg = "Error rendering " + birtContentType + " attachment for email: " + ee.toString();
>>>>> Debug.logError(ee, errMsg, module);
>>>>> return ServiceUtil.returnError(errMsg);
>>>>> } catch (SQLException se) {
>>>>> String errMsg = "Error SQL rendering " + birtContentType + " attachment for email: " + se.toString();
>>>>> Debug.logError(se, errMsg, module);
>>>>> return ServiceUtil.returnError(errMsg);
>>>>> }
>>>>>
>>>>> I think the
>>>>> exception.toString()
>>>>> Debug.logError(exception, errMsg, module);
>>>>> information is enough. This for 4 reasons:
>>>>>
>>>>> 1. exception.toString(), and exception in Debug.logError give enough information. No needs to specify things like
>>>>> "Error I/O rendering "
>>>>> "Error FOP rendering"
>>>>> etc.
>>>>> this will anyway be given by the thrown exception
>>>>> 2. It's more legible
>>>>> 3. It's an antipattern people C/P (like I did) so it spreads everywhere in code
>>>>> 4. Those kinds of exception are exceptionally thrown. BTW I personaly believe checked exceptions in Java is not really the best part
>>>>> of the language. As soons as it has spread in library and at large code, you have to suffer all those try/catch blocks
>>>>>
>>>>> So this like below, this can be rewritten
>>>>>
>>>>> } catch (Exception e) {
>>>>> String errMsg = "Error rendering " + birtContentType + " attachment for email: " + e.toString();
>>>>> Debug.logError(e, errMsg, module);
>>>>> return ServiceUtil.returnError(errMsg);
>>>>> }
>>>>>
>>>> The "+ e.toString()" is not needed because Debug will print the Exception's message anyway. I usually delete those because they make the log harder to read.
>>>> -Adrian
>>>
>>> Yes indeed, when you use the ternary version of Debug.log like in "Debug.logError(e, errMsg, module);"
>>> Another thing we can do to clean the error stack in log...
>>>
>>> Jacques
>>
>>
>> 1. There's nothing wrong with swallowing exceptions if the situation calls for it
>> 2. I was merely pointing out that you can't simply take Adrian's comment and apply it everywhere as some golden rule. Not only that, most resources I've seen consider it to be an anti-pattern anyway.
>>
>> Regards
>> Scott
>
> About 2. : I consider it an anti-pattern because, in cases like above, it clutters the code uselessly to give redundant information (also given by the exception itself).
> I did not mean an anti-pattern in all situations of course, especially when you want to give more information than only what the exception can tell.
>
> So if nobody disagree, in case like above, I will removed "useless" exceptions catches in current code.
>
> Jacques
I disagree, I was referring to Adrian's suggestion as an anti-pattern. The whole reason I replied is because I knew someone reading his comment would take it upon themselves to begin mass converting the entire codebase, as if some gospel had just been handed down that must be spread throughout the land. One reason not to do it is because you will now be catching all exceptions even the ones you don't know about that may be added to the API in the future and may require different handling. Just because everything is handled in the same manner now doesn't mean that it will be in the future. If we follow this path we won't even know that a new type exception has been added.
The way things are now does absolutely no harm and is safer than what you are proposing, at the mere cost of a few extra lines of code I don't see a need to change them.
Regards
Scott
Re: Try/Catch Patterns (was: svn commit: r1334497 ...)
Posted by Jacques Le Roux <ja...@les7arts.com>.
From: "Scott Gray" <sc...@hotwaxmedia.com>
> On 6/05/2012, at 9:28 PM, Jacques Le Roux wrote:
>
>> From: "Adrian Crum" <ad...@sandglass-software.com>
>>> On 5/6/2012 10:07 AM, Jacques Le Roux wrote:
>>>> Nobody is speaking about swallowing, I hope we have not such things in OFBiz! (though Scott's wait to read the end before
>>>> thinking I did not get you and being upset ;o)
>>>>
>>>> No, it's more about stuff like
>>>>
>>>> } catch (BirtException e) {
>>>> throw new ViewHandlerException("Birt Error create engine: " + e.toString(), e);
>>>> } catch (IOException e) {
>>>> throw new ViewHandlerException("Error in the response writer/output stream: " + e.toString(), e);
>>>> } catch (SQLException e) {
>>>> throw new ViewHandlerException("get connection error: " + e.toString(), e);
>>>> } catch (GenericEntityException e) {
>>>> throw new ViewHandlerException("generic entity error: " + e.toString(), e);
>>>> } catch (GeneralException e) {
>>>> throw new ViewHandlerException("general error: " + e.toString(), e);
>>>> } catch (SAXException se) {
>>>> String errMsg = "Error SAX rendering " + page + " view handler: " + se.toString();
>>>> Debug.logError(se, errMsg, module);
>>>> throw new ViewHandlerException(errMsg, se);
>>>> } catch (ParserConfigurationException pe) {
>>>> String errMsg = "Error parser rendering " + page + " view handler: " + pe.toString();
>>>> Debug.logError(pe, errMsg, module);
>>>> throw new ViewHandlerException(errMsg, pe);
>>>> }
>>>>
>>>>
>>>> } catch (GeneralException ge) {
>>>> String errMsg = "Error rendering " + birtContentType + " attachment for email: " + ge.toString();
>>>> Debug.logError(ge, errMsg, module);
>>>> return ServiceUtil.returnError(errMsg);
>>>> } catch (IOException ie) {
>>>> String errMsg = "Error I/O rendering " + birtContentType + " attachment for email: " + ie.toString();
>>>> Debug.logError(ie, errMsg, module);
>>>> return ServiceUtil.returnError(errMsg);
>>>> } catch (FOPException fe) {
>>>> String errMsg = "Error FOP rendering " + birtContentType + " attachment for email: " + fe.toString();
>>>> Debug.logError(fe, errMsg, module);
>>>> return ServiceUtil.returnError(errMsg);
>>>> } catch (SAXException se) {
>>>> String errMsg = "Error SAX rendering " + birtContentType + " attachment for email: " + se.toString();
>>>> Debug.logError(se, errMsg, module);
>>>> return ServiceUtil.returnError(errMsg);
>>>> } catch (ParserConfigurationException pe) {
>>>> String errMsg = "Error parser rendering " + birtContentType + " attachment for email: " + pe.toString();
>>>> Debug.logError(pe, errMsg, module);
>>>> return ServiceUtil.returnError(errMsg);
>>>> } catch (EngineException ee) {
>>>> String errMsg = "Error rendering " + birtContentType + " attachment for email: " + ee.toString();
>>>> Debug.logError(ee, errMsg, module);
>>>> return ServiceUtil.returnError(errMsg);
>>>> } catch (SQLException se) {
>>>> String errMsg = "Error SQL rendering " + birtContentType + " attachment for email: " + se.toString();
>>>> Debug.logError(se, errMsg, module);
>>>> return ServiceUtil.returnError(errMsg);
>>>> }
>>>>
>>>> I think the
>>>> exception.toString()
>>>> Debug.logError(exception, errMsg, module);
>>>> information is enough. This for 4 reasons:
>>>>
>>>> 1. exception.toString(), and exception in Debug.logError give enough information. No needs to specify things like
>>>> "Error I/O rendering "
>>>> "Error FOP rendering"
>>>> etc.
>>>> this will anyway be given by the thrown exception
>>>> 2. It's more legible
>>>> 3. It's an antipattern people C/P (like I did) so it spreads everywhere in code
>>>> 4. Those kinds of exception are exceptionally thrown. BTW I personaly believe checked exceptions in Java is not really the best
>>>> part
>>>> of the language. As soons as it has spread in library and at large code, you have to suffer all those try/catch blocks
>>>>
>>>> So this like below, this can be rewritten
>>>>
>>>> } catch (Exception e) {
>>>> String errMsg = "Error rendering " + birtContentType + " attachment for email: " + e.toString();
>>>> Debug.logError(e, errMsg, module);
>>>> return ServiceUtil.returnError(errMsg);
>>>> }
>>>>
>>> The "+ e.toString()" is not needed because Debug will print the Exception's message anyway. I usually delete those because they
>>> make the log harder to read.
>>> -Adrian
>>
>> Yes indeed, when you use the ternary version of Debug.log like in "Debug.logError(e, errMsg, module);"
>> Another thing we can do to clean the error stack in log...
>>
>> Jacques
>
>
> 1. There's nothing wrong with swallowing exceptions if the situation calls for it
> 2. I was merely pointing out that you can't simply take Adrian's comment and apply it everywhere as some golden rule. Not only
> that, most resources I've seen consider it to be an anti-pattern anyway.
>
> Regards
> Scott
About 2. : I consider it an anti-pattern because, in cases like above, it clutters the code uselessly to give redundant information
(also given by the exception itself).
I did not mean an anti-pattern in all situations of course, especially when you want to give more information than only what the
exception can tell.
So if nobody disagree, in case like above, I will removed "useless" exceptions catches in current code.
Jacques
Re: Try/Catch Patterns (was: svn commit: r1334497 ...)
Posted by Scott Gray <sc...@hotwaxmedia.com>.
On 6/05/2012, at 9:28 PM, Jacques Le Roux wrote:
> From: "Adrian Crum" <ad...@sandglass-software.com>
>> On 5/6/2012 10:07 AM, Jacques Le Roux wrote:
>>> Nobody is speaking about swallowing, I hope we have not such things in OFBiz! (though Scott's wait to read the end before thinking I did not get you and being upset ;o)
>>>
>>> No, it's more about stuff like
>>>
>>> } catch (BirtException e) {
>>> throw new ViewHandlerException("Birt Error create engine: " + e.toString(), e);
>>> } catch (IOException e) {
>>> throw new ViewHandlerException("Error in the response writer/output stream: " + e.toString(), e);
>>> } catch (SQLException e) {
>>> throw new ViewHandlerException("get connection error: " + e.toString(), e);
>>> } catch (GenericEntityException e) {
>>> throw new ViewHandlerException("generic entity error: " + e.toString(), e);
>>> } catch (GeneralException e) {
>>> throw new ViewHandlerException("general error: " + e.toString(), e);
>>> } catch (SAXException se) {
>>> String errMsg = "Error SAX rendering " + page + " view handler: " + se.toString();
>>> Debug.logError(se, errMsg, module);
>>> throw new ViewHandlerException(errMsg, se);
>>> } catch (ParserConfigurationException pe) {
>>> String errMsg = "Error parser rendering " + page + " view handler: " + pe.toString();
>>> Debug.logError(pe, errMsg, module);
>>> throw new ViewHandlerException(errMsg, pe);
>>> }
>>>
>>>
>>> } catch (GeneralException ge) {
>>> String errMsg = "Error rendering " + birtContentType + " attachment for email: " + ge.toString();
>>> Debug.logError(ge, errMsg, module);
>>> return ServiceUtil.returnError(errMsg);
>>> } catch (IOException ie) {
>>> String errMsg = "Error I/O rendering " + birtContentType + " attachment for email: " + ie.toString();
>>> Debug.logError(ie, errMsg, module);
>>> return ServiceUtil.returnError(errMsg);
>>> } catch (FOPException fe) {
>>> String errMsg = "Error FOP rendering " + birtContentType + " attachment for email: " + fe.toString();
>>> Debug.logError(fe, errMsg, module);
>>> return ServiceUtil.returnError(errMsg);
>>> } catch (SAXException se) {
>>> String errMsg = "Error SAX rendering " + birtContentType + " attachment for email: " + se.toString();
>>> Debug.logError(se, errMsg, module);
>>> return ServiceUtil.returnError(errMsg);
>>> } catch (ParserConfigurationException pe) {
>>> String errMsg = "Error parser rendering " + birtContentType + " attachment for email: " + pe.toString();
>>> Debug.logError(pe, errMsg, module);
>>> return ServiceUtil.returnError(errMsg);
>>> } catch (EngineException ee) {
>>> String errMsg = "Error rendering " + birtContentType + " attachment for email: " + ee.toString();
>>> Debug.logError(ee, errMsg, module);
>>> return ServiceUtil.returnError(errMsg);
>>> } catch (SQLException se) {
>>> String errMsg = "Error SQL rendering " + birtContentType + " attachment for email: " + se.toString();
>>> Debug.logError(se, errMsg, module);
>>> return ServiceUtil.returnError(errMsg);
>>> }
>>>
>>> I think the
>>> exception.toString()
>>> Debug.logError(exception, errMsg, module);
>>> information is enough. This for 4 reasons:
>>>
>>> 1. exception.toString(), and exception in Debug.logError give enough information. No needs to specify things like
>>> "Error I/O rendering "
>>> "Error FOP rendering"
>>> etc.
>>> this will anyway be given by the thrown exception
>>> 2. It's more legible
>>> 3. It's an antipattern people C/P (like I did) so it spreads everywhere in code
>>> 4. Those kinds of exception are exceptionally thrown. BTW I personaly believe checked exceptions in Java is not really the best part
>>> of the language. As soons as it has spread in library and at large code, you have to suffer all those try/catch blocks
>>>
>>> So this like below, this can be rewritten
>>>
>>> } catch (Exception e) {
>>> String errMsg = "Error rendering " + birtContentType + " attachment for email: " + e.toString();
>>> Debug.logError(e, errMsg, module);
>>> return ServiceUtil.returnError(errMsg);
>>> }
>>>
>> The "+ e.toString()" is not needed because Debug will print the Exception's message anyway. I usually delete those because they make the log harder to read.
>> -Adrian
>
> Yes indeed, when you use the ternary version of Debug.log like in "Debug.logError(e, errMsg, module);"
> Another thing we can do to clean the error stack in log...
>
> Jacques
1. There's nothing wrong with swallowing exceptions if the situation calls for it
2. I was merely pointing out that you can't simply take Adrian's comment and apply it everywhere as some golden rule. Not only that, most resources I've seen consider it to be an anti-pattern anyway.
Regards
Scott
Re: Try/Catch Patterns (was: svn commit: r1334497 ...)
Posted by Jacques Le Roux <ja...@les7arts.com>.
From: "Adrian Crum" <ad...@sandglass-software.com>
> On 5/6/2012 10:07 AM, Jacques Le Roux wrote:
>> Nobody is speaking about swallowing, I hope we have not such things in
>> OFBiz! (though Scott's wait to read the end before thinking I did not
>> get you and being upset ;o)
>>
>> No, it's more about stuff like
>>
>> } catch (BirtException e) {
>> throw new ViewHandlerException("Birt Error create engine: " +
>> e.toString(), e);
>> } catch (IOException e) {
>> throw new ViewHandlerException("Error in the response writer/output
>> stream: " + e.toString(), e);
>> } catch (SQLException e) {
>> throw new ViewHandlerException("get connection error: " +
>> e.toString(), e);
>> } catch (GenericEntityException e) {
>> throw new ViewHandlerException("generic entity error: " +
>> e.toString(), e);
>> } catch (GeneralException e) {
>> throw new ViewHandlerException("general error: " + e.toString(), e);
>> } catch (SAXException se) {
>> String errMsg = "Error SAX rendering " + page + " view handler: " +
>> se.toString();
>> Debug.logError(se, errMsg, module);
>> throw new ViewHandlerException(errMsg, se);
>> } catch (ParserConfigurationException pe) {
>> String errMsg = "Error parser rendering " + page + " view handler:
>> " + pe.toString();
>> Debug.logError(pe, errMsg, module);
>> throw new ViewHandlerException(errMsg, pe);
>> }
>>
>>
>> } catch (GeneralException ge) {
>> String errMsg = "Error rendering " + birtContentType + " attachment
>> for email: " + ge.toString();
>> Debug.logError(ge, errMsg, module);
>> return ServiceUtil.returnError(errMsg);
>> } catch (IOException ie) {
>> String errMsg = "Error I/O rendering " + birtContentType + "
>> attachment for email: " + ie.toString();
>> Debug.logError(ie, errMsg, module);
>> return ServiceUtil.returnError(errMsg);
>> } catch (FOPException fe) {
>> String errMsg = "Error FOP rendering " + birtContentType + "
>> attachment for email: " + fe.toString();
>> Debug.logError(fe, errMsg, module);
>> return ServiceUtil.returnError(errMsg);
>> } catch (SAXException se) {
>> String errMsg = "Error SAX rendering " + birtContentType + "
>> attachment for email: " + se.toString();
>> Debug.logError(se, errMsg, module);
>> return ServiceUtil.returnError(errMsg);
>> } catch (ParserConfigurationException pe) {
>> String errMsg = "Error parser rendering " + birtContentType + "
>> attachment for email: " + pe.toString();
>> Debug.logError(pe, errMsg, module);
>> return ServiceUtil.returnError(errMsg);
>> } catch (EngineException ee) {
>> String errMsg = "Error rendering " + birtContentType + " attachment
>> for email: " + ee.toString();
>> Debug.logError(ee, errMsg, module);
>> return ServiceUtil.returnError(errMsg);
>> } catch (SQLException se) {
>> String errMsg = "Error SQL rendering " + birtContentType + "
>> attachment for email: " + se.toString();
>> Debug.logError(se, errMsg, module);
>> return ServiceUtil.returnError(errMsg);
>> }
>>
>> I think the
>> exception.toString()
>> Debug.logError(exception, errMsg, module);
>> information is enough. This for 4 reasons:
>>
>> 1. exception.toString(), and exception in Debug.logError give enough
>> information. No needs to specify things like
>> "Error I/O rendering "
>> "Error FOP rendering"
>> etc.
>> this will anyway be given by the thrown exception
>> 2. It's more legible
>> 3. It's an antipattern people C/P (like I did) so it spreads
>> everywhere in code
>> 4. Those kinds of exception are exceptionally thrown. BTW I personaly
>> believe checked exceptions in Java is not really the best part
>> of the language. As soons as it has spread in library and at large
>> code, you have to suffer all those try/catch blocks
>>
>> So this like below, this can be rewritten
>>
>> } catch (Exception e) {
>> String errMsg = "Error rendering " + birtContentType + " attachment
>> for email: " + e.toString();
>> Debug.logError(e, errMsg, module);
>> return ServiceUtil.returnError(errMsg);
>> }
>>
>
> The "+ e.toString()" is not needed because Debug will print the
> Exception's message anyway. I usually delete those because they make the
> log harder to read.
>
> -Adrian
Yes indeed, when you use the ternary version of Debug.log like in "Debug.logError(e, errMsg, module);"
Another thing we can do to clean the error stack in log...
Jacques