[GitHub] [fineract] vorburger commented on a change in pull request #926: FINERACT-977

[GitBox <gi...@apache.org>]

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