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 <[email protected]> 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 [email protected]:
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 {
