ofbiz-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jacques Le Roux <jacques.le.r...@les7arts.com>
Subject Re: svn commit: r1807240 - /ofbiz/ofbiz-framework/trunk/applications/order/src/main/java/org/apache/of biz/order/order/OrderReadHelper.java
Date Sun, 10 Sep 2017 17:40:17 GMT
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.

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.

If you have another alternative, I'm ready to discuss it.

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 :)

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.brohl@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 jleroux@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 {
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message