fineract-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From GitBox <...@apache.org>
Subject [GitHub] [fineract] vorburger commented on a change in pull request #926: FINERACT-977
Date Fri, 22 May 2020 19:48:53 GMT

vorburger commented on a change in pull request #926:
URL: https://github.com/apache/fineract/pull/926#discussion_r429425458



##########
File path: fineract-provider/src/main/java/org/apache/fineract/portfolio/loanaccount/loanschedule/domain/AbstractLoanScheduleGenerator.java
##########
@@ -517,9 +517,11 @@ private LoanScheduleModelPeriod handleRecalculationForTransactions(final
MathCon
                                 scheduleParams.getTotalCumulativePrincipal().plus(
                                         currentPeriodParams.getPrincipalForThisPeriod().minus(principalProcessed)),
                                 scheduleParams.getPeriodNumber() + 1, mc));
-                        if (loanApplicationTerms.getAmortizationMethod().isEqualInstallment()
-                                && fixedEmiAmount.compareTo(loanApplicationTerms.getFixedEmiAmount())
!= 0) {
-                            currentPeriodParams.setEmiAmountChanged(true);
+                        if (loanApplicationTerms.getAmortizationMethod().isEqualInstallment())
+                          if(fixedEmiAmount != null) {
+                            if (fixedEmiAmount.compareTo(loanApplicationTerms.getFixedEmiAmount())
!= 0) {

Review comment:
       The 3 nested `if` are not necessary, and this should instead be written like so (correctly
formatted).
   
   ```suggestion
                           if (loanApplicationTerms.getAmortizationMethod().isEqualInstallment()
                             && fixedEmiAmount != null
                             && fixedEmiAmount.compareTo(loanApplicationTerms.getFixedEmiAmount())
!= 0) {
   ```
   
   More importantly, while I don't understand much about this functionality, it seems to me
that as-is we may be missing a subtle corner case... what if one of `fixedEmiAmount` and `loanApplicationTerms.getFixedEmiAmount()`
are `null` but (respectively) the other is not (anymore?) - should that also trigger `setEmiAmountChanged(true)`
? I don't know details about this, but that would kind of seem "logical", to me. If you agree,
then the check should take that case into account.
   
   BTW we are assuming here that it is valid for an "Emi Amount" (whatever that is???) to
be `null`. Again, I don't know anything about this functionality, so unless you know more,
I guess it's a fair assumption. If someone know actually understands the details of this can
chime in here with knowledge if it's expected to (sometimes) be null, or if that is really
an indication of another problem, that could be interesting, but if we don't learn more, let's
just go for making it null safe.
   
   In an ideal world, an integration test for this would be nice, but I'd rather merge it
without one than not having this fixed... ;-)
   
   The same feedback as above should be applied below as well.
   
   Lastly, the commit message here needs to be improved, instead of "Fix for Null Values"
e.g. "fix possible NPE in AbstractLoanScheduleGenerator [FINERACT-977]" would be better.
   
   @pramodn02 according to Git Blame log, these lines of code were introduced by you, long
ago, for [MIFOSX-579 ](https://mifosforge.jira.com/browse/MIFOSX-579)... if you still have
any interest in this project, perhaps you'd like to chime in here!




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



Mime
View raw message