Inline
On Sun, Sep 10, 2017 at 8:40 PM, Jacques Le Roux <jacques.le.r...@les7arts.com> wrote: > I easily see 3 alternatives: > > 1. Swallow the exception, like we currently do. This should be forbidden in > all cases, I'd veto that!- > 2. Log an error and return a wrong result, like it's currently done by > getShippableTotal() in the same class. That's what I proposed, but instead > of > returning ZERO, I returned the already calculated result. > 3. Throw an error, based on Scott's suggestion in dev ML, like the patch > does. As I've mentioned numerous time, i don't recommend mass fixes because you might be fixing an error with another error. Bulk changes are never a good idea because it means you don't know exactly what you're doing or how the code is affected. Such changes need to happen slowly and carefully. Just because because we have swallowed exceptions does not mean we mass fix them. You might trigger other unknown side effects without carefully studying every thing. My recommendation is to not touch anything, maybe a log message would be enough, but not to change anything else without a careful deep look at the code. > > Sincerely I have not strong opinions about 2 and 3. But technically I prefer > 3 because the exception must then be handled by the caller (very unlikely to > be isolated, we talk about a GenericEntityException here). That's how an API > should be written, the exception should pop to the initial topmost caller. Not necessarily, the exception propagation strategy is dependent on the design, architecture and intent. Exception handling can get real nasty real quick if not studied carefully, it will pollute your design and make things painful for everyone. Sometimes it makes sense to escalate an exception, and sometimes it makes sense to handle it immediately, it just depends on the situation. > > If you have another alternative, I'm ready to discuss it. Already suggested above > > Maybe you will suggest to refactor the whole thing (class, classes, etc.) > but sincerely then an elaborated plan would be needed beforehand. > And in any case it's then an order magnitude more complex that the 2 > solutions above. I don't say it's impossible, just that I'll maybe not see > it :) Ahhh, interesting observation, but even I don't know what I would suggest next :) I write things when I think them. > > Jacques > > > > Le 10/09/2017 à 18:22, Taher Alkhateeb a écrit : >> >> What I understand is that the patch is essentially changing the >> signature of most methods to throw an exception. On a first glance >> this seems to be putting the code in a worst state because now you're >> adding complexity for the caller to figure out how to handle these >> exceptions. >> >> What is the purpose of this change? What is the gain? >> >> On Fri, Sep 8, 2017 at 12:26 PM, Michael Brohl <michael.br...@ecomify.de> >> wrote: >>> >>> Thanks, Jacques. >>> >>> Regards, >>> >>> Michael >>> >>> Am 08.09.17 um 10:54 schrieb Jacques Le Roux: >>> >>>> No worries Michael, >>>> >>>> I can wait a week more >>>> >>>> Jacques >>>> >>>> >>>> Le 08/09/2017 à 10:42, Michael Brohl a écrit : >>>>> >>>>> I disagree, not on the patch itself but on the time frame you give us >>>>> to >>>>> review and think about it. >>>>> >>>>> I see no reason to put pressure on this issue. >>>>> >>>>> Regards, >>>>> >>>>> Michael >>>>> >>>>> >>>>> Am 08.09.17 um 10:02 schrieb Jacques Le Roux: >>>>>> >>>>>> If nobody disagree my last comments at OFBIZ-8341 I will commit the >>>>>> "OFBIZ-8341- OrderReadHelper.patch" this weekend >>>>>> >>>>>> Jacques >>>>>> >>>>>> >>>>>> Le 05/09/2017 à 07:31, Jacques Le Roux a écrit : >>>>>>> >>>>>>> Yes thanks Michael, >>>>>>> >>>>>>> I agree with Scott about rather throwing an exception >>>>>>> >>>>>>> Jacques >>>>>>> >>>>>>> >>>>>>> Le 04/09/2017 à 21:28, Michael Brohl a écrit : >>>>>>>> >>>>>>>> Hi Jacques, >>>>>>>> >>>>>>>> I think directly returning the result inside the catch block changes >>>>>>>> the logic of the code (the adjustments are not added). >>>>>>>> >>>>>>>> Please have another look. >>>>>>>> >>>>>>>> Thanks, >>>>>>>> >>>>>>>> Michael >>>>>>>> >>>>>>>> >>>>>>>> Am 04.09.17 um 17:12 schrieb jler...@apache.org: >>>>>>>>> >>>>>>>>> Author: jleroux >>>>>>>>> Date: Mon Sep 4 15:12:23 2017 >>>>>>>>> New Revision: 1807240 >>>>>>>>> >>>>>>>>> URL: http://svn.apache.org/viewvc?rev=1807240&view=rev >>>>>>>>> Log: >>>>>>>>> Fixed: Fix Default or Empty Catch block in Java and Groovy files >>>>>>>>> (OFBIZ-8341) >>>>>>>>> >>>>>>>>> In many Java and Groovy files we have auto generated catch blocks >>>>>>>>> or >>>>>>>>> empty catch >>>>>>>>> blocks. >>>>>>>>> To avoid such exception swallowing this should be improved to at >>>>>>>>> least log the >>>>>>>>> error and also return error in case of service. >>>>>>>>> >>>>>>>>> jleroux: I can't see what we could do more here, unlikely anyway >>>>>>>>> >>>>>>>>> Modified: >>>>>>>>> >>>>>>>>> >>>>>>>>> ofbiz/ofbiz-framework/trunk/applications/order/src/main/java/org/apache/ofbiz/order/order/OrderReadHelper.java >>>>>>>>> >>>>>>>>> Modified: >>>>>>>>> >>>>>>>>> ofbiz/ofbiz-framework/trunk/applications/order/src/main/java/org/apache/ofbiz/order/order/OrderReadHelper.java >>>>>>>>> URL: >>>>>>>>> >>>>>>>>> http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/applications/order/src/main/java/org/apache/ofbiz/order/order/OrderReadHelper.java?rev=1807240&r1=1807239&r2=1807240&view=diff >>>>>>>>> >>>>>>>>> >>>>>>>>> ============================================================================== >>>>>>>>> --- >>>>>>>>> >>>>>>>>> ofbiz/ofbiz-framework/trunk/applications/order/src/main/java/org/apache/ofbiz/order/order/OrderReadHelper.java >>>>>>>>> (original) >>>>>>>>> +++ >>>>>>>>> >>>>>>>>> ofbiz/ofbiz-framework/trunk/applications/order/src/main/java/org/apache/ofbiz/order/order/OrderReadHelper.java >>>>>>>>> Mon Sep 4 15:12:23 2017 >>>>>>>>> @@ -2414,10 +2414,13 @@ public class OrderReadHelper { >>>>>>>>> List<GenericValue> workOrderItemFulfillments = >>>>>>>>> null; >>>>>>>>> try { >>>>>>>>> workOrderItemFulfillments = >>>>>>>>> orderItem.getDelegator().findByAnd("WorkOrderItemFulfillment", >>>>>>>>> UtilMisc.toMap("orderId", orderItem.getString("orderId"), >>>>>>>>> "orderItemSeqId", >>>>>>>>> orderItem.getString("orderItemSeqId")), null, true); >>>>>>>>> - } catch (GenericEntityException e) {} >>>>>>>>> + } catch (GenericEntityException e) { >>>>>>>>> + Debug.logError(e, module); >>>>>>>>> + return result; >>>>>>>>> + } >>>>>>>>> if (workOrderItemFulfillments != null) { >>>>>>>>> Iterator<GenericValue> iter = >>>>>>>>> workOrderItemFulfillments.iterator(); >>>>>>>>> - if (iter.hasNext()) { >>>>>>>>> + if (iter.hasNext()) { >>>>>>>>> GenericValue WorkOrderItemFulfillment = >>>>>>>>> iter.next(); >>>>>>>>> GenericValue workEffort = null; >>>>>>>>> try { >>>>>>>>> >>>>>>>>> >>>>>>>> >>>>>>> >
