ptuomola commented on a change in pull request #1006:
URL: https://github.com/apache/fineract/pull/1006#discussion_r444616087
##########
File path:
fineract-provider/src/main/java/org/apache/fineract/portfolio/loanaccount/domain/Loan.java
##########
@@ -4501,7 +4505,9 @@ private LocalDate getMaxDateLimitForNewRepayment(final
PeriodFrequencyType perio
break;
case INVALID:
break;
- default:
Review comment:
As far as I can see, there are three options for this WHOLE_TERM logic
(and other "missing cases"):
1. These functions are not needed when enum is set to WHOLE_TERM and are
never called with that value (e.g. there is no need to determine max date for
repayment when you are paying for the whole term in one go). In that case the
logic for WHOLE_TERM is not needed in the functions and we should not log an
error as there is nothing to be done.
2. These functions are needed when enum is set to WHOLE_TERM and are called
with that value. However the default logic - i.e. do nothing in the switch -
results in the function returning the right value. If that's the case, then we
should just make this explicit: add a case for WHOLE_TERM that does nothing,
and a comment that this has been added for a reason. But no need to log an
error as the "do nothing" behaviour is correct.
3. These functions are needed when enum is set to WHOLE_TERM but the logic
to handle WHOLE_TERM has not been implemented. If that's the case, the function
is currently returning an incorrect value when called with WHOLE_TERM. We
should then add the case and a log statement to output a TODO to indicate that
the logic is missing.
From response of @percyashu to my earlier question, I understood that in
this case the answer is either #1 or #2 - so logging nothing is correct. But in
other switch statements in the code, the answer may be #3 so logging an error /
TODO would be correct.
In my view, to see if this PR is correct, we'd need to categorise each of
the switch statements to one of the above cases. I started doing that and went
through them once earlier, but there are a couple of cases (like this
WHOLE_TERM) where it's difficult to be sure just by looking at the code...
----------------------------------------------------------------
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:
[email protected]
