[jira] [Comment Edited] (OFBIZ-8341) Fix Default or Empty Catch block in Java and Groovy files

[Nicolas Malin (Jira)]

    [ https://issues.apache.org/jira/browse/OFBIZ-8341?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16153202#comment-16153202 ]

Jacques Le Roux edited comment on OFBIZ-8341 at 9/5/17 7:25 AM:
----------------------------------------------------------------

Also it's worth to note here that arguably the same kind of exception handling that I did in r1807240 is done in getShippableTotal() (even worse since in a loop):
{code}
    public BigDecimal getShippableTotal(String shipGroupSeqId) {
        BigDecimal shippableTotal = ZERO;
        List<GenericValue> validItems = getValidOrderItems(shipGroupSeqId);
        if (validItems != null) {
            for (GenericValue item : validItems) {
                GenericValue product = null;
                try {
                    product = item.getRelatedOne("Product", false);
                } catch (GenericEntityException e) {
                    Debug.logError(e, "Problem getting Product from OrderItem; returning 0", module);
                    return ZERO;
                }
                if (product != null) {
                    if (ProductWorker.shippingApplies(product)) {
                        shippableTotal = shippableTotal.add(OrderReadHelper.getOrderItemSubTotal(item, getAdjustments(), false, true)).setScale(scale, rounding);
                    }
                }
            }
        }
        return shippableTotal.setScale(scale, rounding);
    }
{code}
Theoretically it's also better here to throw an exception rather than hiding it by returning an unknown value (here ZERO, we already spoke about this "ZERO" BTW)

I say theoretically because as Scott stated in his message on dev ML
bq. Also worth noting the reality in this case, if your system is throwing EntityExceptions from a simple db read then whatever you're trying accomplish is already going to error out somewhere along the chain before or after this method is called.
But this should not prevent us to write the best possible code, even if it's theoretically in case of EntityException (something worse is happening underneath anyway)


was (Author: jacques.le.roux):
Also it's worth to note here that arguably the same kind of exception handling that I did in r1807240 is done in getShippableTotal() (even worse since in a loop):
{code}
    public BigDecimal getShippableTotal(String shipGroupSeqId) {
        BigDecimal shippableTotal = ZERO;
        List<GenericValue> validItems = getValidOrderItems(shipGroupSeqId);
        if (validItems != null) {
            for (GenericValue item : validItems) {
                GenericValue product = null;
                try {
                    product = item.getRelatedOne("Product", false);
                } catch (GenericEntityException e) {
                    Debug.logError(e, "Problem getting Product from OrderItem; returning 0", module);
                    return ZERO;
                }
                if (product != null) {
                    if (ProductWorker.shippingApplies(product)) {
                        shippableTotal = shippableTotal.add(OrderReadHelper.getOrderItemSubTotal(item, getAdjustments(), false, true)).setScale(scale, rounding);
                    }
                }
            }
        }
        return shippableTotal.setScale(scale, rounding);
    }
{code}
Theoretically it's also better here to throw an exception rather than hiding it by returning an unknow value (here ZERO, we already spoke about this "ZERO" BTW)

I say theoretically because as Scott stated in his message on dev ML
bq. Also worth noting the reality in this case, if your system is throwing EntityExceptions from a simple db read then whatever you're trying accomplish is already going to error out somewhere along the chain before or after this method is called.
But this should not prevent us to write the best possible code, even if it's theoretically in case of EntityException (something worse is happening underneath anyway)

> Fix Default or Empty Catch block in Java and Groovy files
> ---------------------------------------------------------
>
>                 Key: OFBIZ-8341
>                 URL: https://issues.apache.org/jira/browse/OFBIZ-8341
>             Project: OFBiz
>          Issue Type: Bug
>          Components: ALL COMPONENTS
>            Reporter: Harsh Vijaywargiya
>            Assignee: Jacques Le Roux
>             Fix For: Upcoming Release
>
>         Attachments: OFBIZ-8341- OrderReadHelper.patch
>
>
> 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.
> As of now I found following classes with such patterns -
> InventoryServices.java
> FreeMarkerWorker.java
> QRCodeEvents.java
> QRCodeServices.java

--
This message was sent by Atlassian JIRA
(v6.4.14#64029)