Re: svn commit: r1334497 - in /ofbiz/trunk: applications/accounting/src/org/ofbiz/accounting/thirdparty/clearcommerce/ applications/product/src/org/ofbiz/shipment/thirdparty/fedex/ applications/product/src/org/ofbiz/shipment/thirdparty/usps/ specialpurpose

[Jacques Le Roux]

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


Of course I don't mean this as a general common pattern. It only apply to cases where the 
"same" thing (of course with specific
information proper to the exception thrown) is finally rendered when the 
exception is thrown (see more arguments at bottom)

Quoting 
http://docs.oracle.com/javase/tutorial/essential/exceptions/advantages.html 
(***are mine to underline***)

<<You could even set up an exception handler that handles any Exception with 
the handler here.
// A (too) general exception handler
catch (Exception e) {
   ...
}
The Exception class is close to the top of the Throwable class hierarchy. 
Therefore, this handler will catch many other exceptions
in addition to those that the handler is intended to catch. ***You may want to 
handle exceptions this way if all you want your
program to do, for example, is print out an error message for the user and then 
exit.***

In most situations, however, you want exception handlers to be as specific as 
possible. The reason is that the first thing a handler
must do is determine what type of exception occurred before it can decide on 
the best recovery strategy. In effect, by not catching
specific errors, the handler must accommodate any possibility. Exception 
handlers that are too general can make code more
error-prone by catching and handling exceptions that weren't anticipated by the 
programmer and for which the handler was not
intended.>>

To be back to Scott's objection (I believe he referred to same Objection than 
expressed by Oracle, which makes sense) I believe
anyway that in cases above (from OFBiz) we will give enough information if even 
an unanticipated exception is thrown. So we could
adopt this pattern where the anti-pattern above is used (DRY principle). In our case, 
in <<print out an error message for the user
and then exit>> the user is actually the developer reading the stack, we are 
not specifically handling the type of exception by the
calling method.

If we agree, we could not only recommend this pattern but also clean current 
code, as I already begun.

Opinions?

Jacques

From: "Scott Gray" <scott.g...@hotwaxmedia.com>
I think it depends on what is being done with the exceptions, if you're 
swallowing them then I don't think it would be such a good
idea.

Regards
Scott

On 6/05/2012, at 8:55 AM, Adrian Crum <adrian.c...@sandglass-software.com> 
wrote:

From my perspective, a bunch of catch blocks that all do the exact same thing 
can be consolidated into a single catch block.

-Adrian

On 5/5/2012 9:46 PM, jler...@apache.org wrote:
Author: jleroux
Date: Sat May  5 20:46:39 2012
New Revision: 1334497

URL: http://svn.apache.org/viewvc?rev=1334497&view=rev
Log:
Non functional changes.
Removes a bunch of useless exceptions catches. There are still few cases to 
discuss, I believe there are useless (or at least
without much more information) as well. we will decide on dev ML later...

Modified:
    
ofbiz/trunk/applications/accounting/src/org/ofbiz/accounting/thirdparty/clearcommerce/CCPaymentServices.java
    
ofbiz/trunk/applications/product/src/org/ofbiz/shipment/thirdparty/fedex/FedexServices.java
    
ofbiz/trunk/applications/product/src/org/ofbiz/shipment/thirdparty/usps/UspsMockApiServlet.java
    
ofbiz/trunk/applications/product/src/org/ofbiz/shipment/thirdparty/usps/UspsServices.java
    
ofbiz/trunk/specialpurpose/workflow/src/org/ofbiz/workflow/WfApplicationServices.java

Modified: 
ofbiz/trunk/applications/accounting/src/org/ofbiz/accounting/thirdparty/clearcommerce/CCPaymentServices.java
URL:
http://svn.apache.org/viewvc/ofbiz/trunk/applications/accounting/src/org/ofbiz/accounting/thirdparty/clearcommerce/CCPaymentServices.java?rev=1334497&r1=1334496&r2=1334497&view=diff
==============================================================================
--- 
ofbiz/trunk/applications/accounting/src/org/ofbiz/accounting/thirdparty/clearcommerce/CCPaymentServices.java
 (original)
+++ 
ofbiz/trunk/applications/accounting/src/org/ofbiz/accounting/thirdparty/clearcommerce/CCPaymentServices.java
 Sat May  5
20:46:39 2012
@@ -19,7 +19,6 @@
 package org.ofbiz.accounting.thirdparty.clearcommerce;

 import java.io.ByteArrayOutputStream;
-import java.io.IOException;
 import java.io.OutputStream;
 import java.math.BigDecimal;
 import java.util.ArrayList;
@@ -28,7 +27,6 @@ import java.util.List;
 import java.util.Locale;
 import java.util.Map;

-import javax.xml.parsers.ParserConfigurationException;
 import javax.xml.transform.TransformerException;

 import org.ofbiz.accounting.payment.PaymentGatewayServices;
@@ -47,7 +45,6 @@ import org.ofbiz.service.DispatchContext
 import org.ofbiz.service.ServiceUtil;
 import org.w3c.dom.Document;
 import org.w3c.dom.Element;
-import org.xml.sax.SAXException;


 /**
@@ -921,12 +918,8 @@ public class CCPaymentServices {
         Document responseDocument = null;
         try {
             responseDocument = UtilXml.readXmlDocument(response, false);
-        } catch (SAXException se) {
-            throw new ClearCommerceException("Error reading response Document from 
a String: " + se.getMessage());
-        } catch (ParserConfigurationException pce) {
-            throw new ClearCommerceException("Error reading response Document from 
a String: " + pce.getMessage());
-        } catch (IOException ioe) {
-            throw new ClearCommerceException("Error reading response Document from 
a String: " + ioe.getMessage());
+        } catch (Exception e) {
+            throw new ClearCommerceException("Error reading response Document from 
a String: " + e.getMessage());
         }
         if (Debug.verboseOn()) Debug.logVerbose("Result severity from 
clearCommerce:" + getMessageListMaxSev(responseDocument),
module);
         if (Debug.verboseOn()&&  getMessageListMaxSev(responseDocument)>  4)

Modified: 
ofbiz/trunk/applications/product/src/org/ofbiz/shipment/thirdparty/fedex/FedexServices.java
URL:
http://svn.apache.org/viewvc/ofbiz/trunk/applications/product/src/org/ofbiz/shipment/thirdparty/fedex/FedexServices.java?rev=1334497&r1=1334496&r2=1334497&view=diff
==============================================================================
--- 
ofbiz/trunk/applications/product/src/org/ofbiz/shipment/thirdparty/fedex/FedexServices.java
 (original)
+++ 
ofbiz/trunk/applications/product/src/org/ofbiz/shipment/thirdparty/fedex/FedexServices.java
 Sat May  5 20:46:39 2012
@@ -19,7 +19,6 @@

 package org.ofbiz.shipment.thirdparty.fedex;

-import java.io.IOException;
 import java.io.StringWriter;
 import java.math.BigDecimal;
 import java.sql.Timestamp;
@@ -27,8 +26,6 @@ import java.util.List;
 import java.util.Locale;
 import java.util.Map;

-import javax.xml.parsers.ParserConfigurationException;
-
 import javolution.util.FastList;
 import javolution.util.FastMap;

@@ -58,7 +55,6 @@ import org.ofbiz.service.ServiceUtil;
 import org.ofbiz.shipment.shipment.ShipmentServices;
 import org.w3c.dom.Document;
 import org.w3c.dom.Element;
-import org.xml.sax.SAXException;

 /**
  * Fedex Shipment Services
@@ -364,24 +360,12 @@ public class FedexServices {
             try {
                 fDXSubscriptionReplyDocument = 
UtilXml.readXmlDocument(fDXSubscriptionReplyString, false);
                 Debug.logInfo("Fedex response for FDXSubscriptionRequest:" + 
fDXSubscriptionReplyString, module);
-            } catch (SAXException se) {
-                String errorMessage = "Error parsing the FDXSubscriptionRequest 
response: " + se.toString();
-                Debug.logError(se, errorMessage, module);
-                return 
ServiceUtil.returnError(UtilProperties.getMessage(resourceError,
-                        
"FacilityShipmentFedexSubscriptionTemplateParsingError",
-                        UtilMisc.toMap("errorString", se.toString()), locale));
-            } catch (ParserConfigurationException pce) {
-                String errorMessage = "Error parsing the FDXSubscriptionRequest 
response: " + pce.toString();
-                Debug.logError(pce, errorMessage, module);
-                return 
ServiceUtil.returnError(UtilProperties.getMessage(resourceError,
-                        
"FacilityShipmentFedexSubscriptionTemplateParsingError",
-                        UtilMisc.toMap("errorString", pce.toString()), 
locale));
-            } catch (IOException ioe) {
-                String errorMessage = "Error parsing the FDXSubscriptionRequest 
response: " + ioe.toString();
-                Debug.logError(ioe, errorMessage, module);
+            } catch (Exception e) {
+                String errorMessage = "Error parsing the FDXSubscriptionRequest 
response: " + e.toString();
+                Debug.logError(e, errorMessage, module);
                 return 
ServiceUtil.returnError(UtilProperties.getMessage(resourceError,
                         
"FacilityShipmentFedexSubscriptionTemplateParsingError",
-                        UtilMisc.toMap("errorString", ioe.toString()), 
locale));
+                        UtilMisc.toMap("errorString", e.toString()), locale));
             }

             Element fedexSubscriptionReplyElement = 
fDXSubscriptionReplyDocument.getDocumentElement();
@@ -996,17 +980,9 @@ public class FedexServices {
         Document fdxShipReplyDocument = null;
         try {
             fdxShipReplyDocument = UtilXml.readXmlDocument(fDXShipReplyString, 
false);
-        } catch (SAXException se) {
-            String errorMessage = "Error parsing the FDXShipReply: " + 
se.toString();
-            Debug.logError(se, errorMessage, module);
-            // TODO: Cancel the package
-        } catch (ParserConfigurationException pe) {
-            String errorMessage = "Error parsing the FDXShipReply: " + 
pe.toString();
-            Debug.logError(pe, errorMessage, module);
-            // TODO Cancel the package
-        } catch (IOException ioe) {
-            String errorMessage = "Error parsing the FDXShipReply: " + 
ioe.toString();
-            Debug.logError(ioe, errorMessage, module);
+        } catch (Exception e) {
+            String errorMessage = "Error parsing the FDXShipReply: " + 
e.toString();
+            Debug.logError(e, errorMessage, module);
             // TODO Cancel the package
         }


Modified: 
ofbiz/trunk/applications/product/src/org/ofbiz/shipment/thirdparty/usps/UspsMockApiServlet.java
URL:
http://svn.apache.org/viewvc/ofbiz/trunk/applications/product/src/org/ofbiz/shipment/thirdparty/usps/UspsMockApiServlet.java?rev=1334497&r1=1334496&r2=1334497&view=diff
==============================================================================
--- 
ofbiz/trunk/applications/product/src/org/ofbiz/shipment/thirdparty/usps/UspsMockApiServlet.java
 (original)
+++ 
ofbiz/trunk/applications/product/src/org/ofbiz/shipment/thirdparty/usps/UspsMockApiServlet.java
 Sat May  5 20:46:39 2012
@@ -30,16 +30,13 @@ import javax.servlet.ServletOutputStream
 import javax.servlet.http.HttpServlet;
 import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpServletResponse;
-import javax.xml.parsers.ParserConfigurationException;
 import javax.xml.transform.TransformerException;

+import org.ofbiz.base.util.Debug;
 import org.ofbiz.base.util.UtilValidate;
 import org.ofbiz.base.util.UtilXml;
-import org.ofbiz.base.util.Debug;
-
 import org.w3c.dom.Document;
 import org.w3c.dom.Element;
-import org.xml.sax.SAXException;

 /**
  * USPS Webtools API Mock API Servlet
@@ -77,14 +74,8 @@ public class UspsMockApiServlet extends
         Document requestDocument = null;
         try {
             requestDocument = UtilXml.readXmlDocument(xmlValue, false);
-        } catch (SAXException se) {
-            Debug.logError(se, module);
-            return;
-        } catch (ParserConfigurationException pce) {
-            Debug.logError(pce, module);
-            return;
-        } catch (IOException xmlReadException) {
-            Debug.logError(xmlReadException, module);
+        } catch (Exception e) {
+            Debug.logError(e, module);
             return;
         }


Modified: 
ofbiz/trunk/applications/product/src/org/ofbiz/shipment/thirdparty/usps/UspsServices.java
URL:
http://svn.apache.org/viewvc/ofbiz/trunk/applications/product/src/org/ofbiz/shipment/thirdparty/usps/UspsServices.java?rev=1334497&r1=1334496&r2=1334497&view=diff
==============================================================================
--- 
ofbiz/trunk/applications/product/src/org/ofbiz/shipment/thirdparty/usps/UspsServices.java
 (original)
+++ 
ofbiz/trunk/applications/product/src/org/ofbiz/shipment/thirdparty/usps/UspsServices.java
 Sat May  5 20:46:39 2012
@@ -32,7 +32,6 @@ import java.util.ListIterator;
 import java.util.Locale;
 import java.util.Map;

-import javax.xml.parsers.ParserConfigurationException;
 import javax.xml.transform.TransformerException;

 import javolution.util.FastList;
@@ -67,7 +66,6 @@ import org.ofbiz.shipment.shipment.Shipm
 import org.ofbiz.shipment.shipment.ShipmentWorker;
 import org.w3c.dom.Document;
 import org.w3c.dom.Element;
-import org.xml.sax.SAXException;

 /**
  * USPS Webtools API Services
@@ -1757,18 +1755,8 @@ public class UspsServices {
         Document responseDocument = null;
         try {
             responseDocument = UtilXml.readXmlDocument(responseString, false);
-        } catch (SAXException se) {
-            throw new 
UspsRequestException(UtilProperties.getMessage(resourceError,
-                    "FacilityShipmentUspsResponseError",
-                    UtilMisc.toMap("errorString", se.getMessage()), locale));
-        } catch (ParserConfigurationException pce) {
-            throw new 
UspsRequestException(UtilProperties.getMessage(resourceError,
-                    "FacilityShipmentUspsResponseError",
-                    UtilMisc.toMap("errorString", pce.getMessage()), locale));
-        } catch (IOException xmlReadException) {
-            throw new 
UspsRequestException(UtilProperties.getMessage(resourceError,
-                    "FacilityShipmentUspsResponseError",
-                    UtilMisc.toMap("errorString", 
xmlReadException.getMessage()), locale));
+        } catch (Exception e) {
+            throw new UspsRequestException(UtilProperties.getMessage(resourceError, 
"FacilityShipmentUspsResponseError",
UtilMisc.toMap("errorString", e.getMessage()), locale));
         }

         // If a top-level error document is returned, throw exception

Modified: 
ofbiz/trunk/specialpurpose/workflow/src/org/ofbiz/workflow/WfApplicationServices.java
URL:
http://svn.apache.org/viewvc/ofbiz/trunk/specialpurpose/workflow/src/org/ofbiz/workflow/WfApplicationServices.java?rev=1334497&r1=1334496&r2=1334497&view=diff
==============================================================================
--- 
ofbiz/trunk/specialpurpose/workflow/src/org/ofbiz/workflow/WfApplicationServices.java
 (original)
+++ 
ofbiz/trunk/specialpurpose/workflow/src/org/ofbiz/workflow/WfApplicationServices.java
 Sat May  5 20:46:39 2012
@@ -25,8 +25,6 @@ import java.util.HashMap;
 import java.util.List;
 import java.util.Map;

-import javax.xml.parsers.ParserConfigurationException;
-
 import org.ofbiz.base.util.Debug;
 import org.ofbiz.base.util.GeneralException;
 import org.ofbiz.base.util.ObjectType;
@@ -41,7 +39,6 @@ import org.ofbiz.entity.serialize.XmlSer
 import org.ofbiz.service.DispatchContext;
 import org.ofbiz.service.GenericServiceException;
 import org.ofbiz.service.ModelService;
-import org.xml.sax.SAXException;

 /**
  * Workflow Application Services - 'Services' and 'Workers' for interaction 
with Workflow Application API
@@ -219,14 +216,8 @@ public class WfApplicationServices {
             throws GenericServiceException {
         try {
             return UtilGenerics.checkMap(XmlSerializer.deserialize((String) 
runTimeData.get("runtimeInfo"), delegator));
-        } catch (SerializeException se) {
-            throw new GenericServiceException(se.getMessage(), se);
-        } catch (ParserConfigurationException pe) {
-            throw new GenericServiceException(pe.getMessage(), pe);
-        } catch (SAXException se) {
-            throw new GenericServiceException(se.getMessage(), se);
-        } catch (IOException ioe) {
-            throw new GenericServiceException(ioe.getMessage(), ioe);
+        } catch (Exception e) {
+            throw new GenericServiceException(e.getMessage(), e);
         }
     }