Re: svn commit: r1804859 - /ofbiz/ofbiz-framework/trunk/applications/accounting/src/main/java/org/apache/ofbiz/accounting/payment/GiftCertificateServices.java

[Gil Portenseigne]

Hello Jacques,

The reason why no assignment is good here, is that the assignment value
will never be used, since the value is always overwritten with another
assignment before it is used.

It's the same with  :

     String refNum = null;

lines : 228 and 305.

Gil


On 12/08/2017 09:59, [hidden email] wrote:

> Author: jleroux
> Date: Sat Aug 12 07:59:16 2017
> New Revision: 1804859
>
> URL: http://svn.apache.org/viewvc?rev=1804859&view=rev
> Log:
> Reverted: [FB] Package org.apache.ofbiz.accounting.payment (Additional Bugs)
> (OFBIZ-9529)
>
> GiftCertificateServices.java:229, DLS_DEAD_LOCAL_STORE
> GiftCertificateServices.java:306, DLS_DEAD_LOCAL_STORE
> DLS: Dead store to balance in
> GiftCertificateServices.addFundsToGiftCertificate(DispatchContext, Map)
> This instruction assigns a value to a local variable, but the value is not read
> or used in any subsequent instruction. Often, this indicates an error,
> because the value computed is never used.
>
> It was late and I was too quick yesterday night. I see no reason why we would
> replace ZERO by null (no assignment) in both cases, the variables are used.
> So I revert r1804847 and I rather replace occurrences of BigDecimal.ZERO by the
> locally defined constant ZERO
>
> Modified:
>      ofbiz/ofbiz-framework/trunk/applications/accounting/src/main/java/org/apache/ofbiz/accounting/payment/GiftCertificateServices.java
>
> Modified: ofbiz/ofbiz-framework/trunk/applications/accounting/src/main/java/org/apache/ofbiz/accounting/payment/GiftCertificateServices.java
> URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/applications/accounting/src/main/java/org/apache/ofbiz/accounting/payment/GiftCertificateServices.java?rev=1804859&r1=1804858&r2=1804859&view=diff
> ==============================================================================
> --- ofbiz/ofbiz-framework/trunk/applications/accounting/src/main/java/org/apache/ofbiz/accounting/payment/GiftCertificateServices.java (original)
> +++ ofbiz/ofbiz-framework/trunk/applications/accounting/src/main/java/org/apache/ofbiz/accounting/payment/GiftCertificateServices.java Sat Aug 12 07:59:16 2017
> @@ -226,7 +226,7 @@ public class GiftCertificateServices {
>           }
>  
>           // create the transaction
> -        BigDecimal balance;
> +        BigDecimal balance = ZERO;
>           String refNum = null;
>           try {
>               refNum = GiftCertificateServices.createTransaction(delegator, dispatcher, userLogin, amount, productStoreId, partyId,
> @@ -270,7 +270,7 @@ public class GiftCertificateServices {
>           }
>  
>           // validate the amount
> -        if (amount.compareTo(BigDecimal.ZERO) < 0) {
> +        if (amount.compareTo(ZERO) < 0) {
>               return ServiceUtil.returnError(UtilProperties.getMessage(resourceError,
>                       "AccountingFinAccountMustBePositive", locale));
>           }
> @@ -301,9 +301,9 @@ public class GiftCertificateServices {
>           }
>  
>           // check the actual balance (excluding authorized amounts) and create the transaction if it is sufficient
> -        BigDecimal previousBalance = finAccount.get("actualBalance") == null ? BigDecimal.ZERO : finAccount.getBigDecimal("actualBalance");
> +        BigDecimal previousBalance = finAccount.get("actualBalance") == null ? ZERO : finAccount.getBigDecimal("actualBalance");
>  
> -        BigDecimal balance;
> +        BigDecimal balance = ZERO;
>           String refNum = null;
>           Boolean procResult;
>           if (previousBalance.compareTo(amount) >= 0) {
> @@ -311,7 +311,7 @@ public class GiftCertificateServices {
>                   refNum = GiftCertificateServices.createTransaction(delegator, dispatcher, userLogin, amount, productStoreId,
>                           partyId, currencyUom, withdrawl, cardNumber, locale);
>                   finAccount.refresh();
> -                balance = finAccount.get("availableBalance") == null ? BigDecimal.ZERO : finAccount.getBigDecimal("availableBalance");
> +                balance = finAccount.get("availableBalance") == null ? ZERO : finAccount.getBigDecimal("availableBalance");
>                   procResult = Boolean.TRUE;
>               } catch (GeneralException e) {
>                   Debug.logError(e, module);
> @@ -356,7 +356,7 @@ public class GiftCertificateServices {
>  
>           // TODO: get the real currency from context
>           // get the balance
> -        BigDecimal balance = finAccount.get("availableBalance") == null ? BigDecimal.ZERO : finAccount.getBigDecimal("availableBalance");
> +        BigDecimal balance = finAccount.get("availableBalance") == null ? ZERO : finAccount.getBigDecimal("availableBalance");
>  
>           Map<String, Object> result = ServiceUtil.returnSuccess();
>           result.put("balance", balance);
>
>

[Jacques Le Roux]

Thanks Gil,

Got it now :)

BTW what do you think about my 1st comment at https://issues.apache.org/jira/browse/OFBIZ-9572

Jacques


Le 14/08/2017 à 16:30, gil portenseigne a écrit :

> Hello Jacques,
>
> The reason why no assignment is good here, is that the assignment value will never be used, since the value is always overwritten with another
> assignment before it is used.
>
> It's the same with  :
>
>     String refNum = null;
>
> lines : 228 and 305.
>
> Gil
>
>
> On 12/08/2017 09:59, [hidden email] wrote:
>> Author: jleroux
>> Date: Sat Aug 12 07:59:16 2017
>> New Revision: 1804859
>>
>> URL: http://svn.apache.org/viewvc?rev=1804859&view=rev
>> Log:
>> Reverted: [FB] Package org.apache.ofbiz.accounting.payment (Additional Bugs)
>> (OFBIZ-9529)
>>
>> GiftCertificateServices.java:229, DLS_DEAD_LOCAL_STORE
>> GiftCertificateServices.java:306, DLS_DEAD_LOCAL_STORE
>> DLS: Dead store to balance in
>> GiftCertificateServices.addFundsToGiftCertificate(DispatchContext, Map)
>> This instruction assigns a value to a local variable, but the value is not read
>> or used in any subsequent instruction. Often, this indicates an error,
>> because the value computed is never used.
>>
>> It was late and I was too quick yesterday night. I see no reason why we would
>> replace ZERO by null (no assignment) in both cases, the variables are used.
>> So I revert r1804847 and I rather replace occurrences of BigDecimal.ZERO by the
>> locally defined constant ZERO
>>
>> Modified:
>> ofbiz/ofbiz-framework/trunk/applications/accounting/src/main/java/org/apache/ofbiz/accounting/payment/GiftCertificateServices.java
>>
>> Modified: ofbiz/ofbiz-framework/trunk/applications/accounting/src/main/java/org/apache/ofbiz/accounting/payment/GiftCertificateServices.java
>> URL:
>> http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/applications/accounting/src/main/java/org/apache/ofbiz/accounting/payment/GiftCertificateServices.java?rev=1804859&r1=1804858&r2=1804859&view=diff
>> ==============================================================================
>> --- ofbiz/ofbiz-framework/trunk/applications/accounting/src/main/java/org/apache/ofbiz/accounting/payment/GiftCertificateServices.java (original)
>> +++ ofbiz/ofbiz-framework/trunk/applications/accounting/src/main/java/org/apache/ofbiz/accounting/payment/GiftCertificateServices.java Sat Aug 12
>> 07:59:16 2017
>> @@ -226,7 +226,7 @@ public class GiftCertificateServices {
>>           }
>>             // create the transaction
>> -        BigDecimal balance;
>> +        BigDecimal balance = ZERO;
>>           String refNum = null;
>>           try {
>>               refNum = GiftCertificateServices.createTransaction(delegator, dispatcher, userLogin, amount, productStoreId, partyId,
>> @@ -270,7 +270,7 @@ public class GiftCertificateServices {
>>           }
>>             // validate the amount
>> -        if (amount.compareTo(BigDecimal.ZERO) < 0) {
>> +        if (amount.compareTo(ZERO) < 0) {
>>               return ServiceUtil.returnError(UtilProperties.getMessage(resourceError,
>>                       "AccountingFinAccountMustBePositive", locale));
>>           }
>> @@ -301,9 +301,9 @@ public class GiftCertificateServices {
>>           }
>>             // check the actual balance (excluding authorized amounts) and create the transaction if it is sufficient
>> -        BigDecimal previousBalance = finAccount.get("actualBalance") == null ? BigDecimal.ZERO : finAccount.getBigDecimal("actualBalance");
>> +        BigDecimal previousBalance = finAccount.get("actualBalance") == null ? ZERO : finAccount.getBigDecimal("actualBalance");
>>   -        BigDecimal balance;
>> +        BigDecimal balance = ZERO;
>>           String refNum = null;
>>           Boolean procResult;
>>           if (previousBalance.compareTo(amount) >= 0) {
>> @@ -311,7 +311,7 @@ public class GiftCertificateServices {
>>                   refNum = GiftCertificateServices.createTransaction(delegator, dispatcher, userLogin, amount, productStoreId,
>>                           partyId, currencyUom, withdrawl, cardNumber, locale);
>>                   finAccount.refresh();
>> -                balance = finAccount.get("availableBalance") == null ? BigDecimal.ZERO : finAccount.getBigDecimal("availableBalance");
>> +                balance = finAccount.get("availableBalance") == null ? ZERO : finAccount.getBigDecimal("availableBalance");
>>                   procResult = Boolean.TRUE;
>>               } catch (GeneralException e) {
>>                   Debug.logError(e, module);
>> @@ -356,7 +356,7 @@ public class GiftCertificateServices {
>>             // TODO: get the real currency from context
>>           // get the balance
>> -        BigDecimal balance = finAccount.get("availableBalance") == null ? BigDecimal.ZERO : finAccount.getBigDecimal("availableBalance");
>> +        BigDecimal balance = finAccount.get("availableBalance") == null ? ZERO : finAccount.getBigDecimal("availableBalance");
>>             Map<String, Object> result = ServiceUtil.returnSuccess();
>>           result.put("balance", balance);
>>
>>
>
>