galovics commented on code in PR #2944:
URL: https://github.com/apache/fineract/pull/2944#discussion_r1095444789
##########
fineract-provider/src/main/java/org/apache/fineract/portfolio/delinquency/service/LoanDelinquencyDomainServiceImpl.java:
##########
@@ -48,12 +48,15 @@ public CollectionData getOverdueCollectionData(final Loan
loan) {
boolean oldestOverdueInstallment = false;
boolean overdueSinceDateWasSet = false;
boolean firstNotYetDueInstallment = false;
+ LoanRepaymentScheduleInstallment latestInstallment =
loan.getRepaymentScheduleInstallments()
Review Comment:
Just from a usability point of view, I think we could create a method on the
loan itself to get the last installment. Seems like a very common use-case to
me and makes this code much more readable.
##########
fineract-provider/src/main/java/org/apache/fineract/portfolio/delinquency/service/LoanDelinquencyDomainServiceImpl.java:
##########
@@ -48,12 +48,15 @@ public CollectionData getOverdueCollectionData(final Loan
loan) {
boolean oldestOverdueInstallment = false;
boolean overdueSinceDateWasSet = false;
boolean firstNotYetDueInstallment = false;
+ LoanRepaymentScheduleInstallment latestInstallment =
loan.getRepaymentScheduleInstallments()
+ .get(loan.getRepaymentScheduleInstallments().size() - 1);
+
+ List<LoanTransaction> chargebackTransactions =
loan.getLoanTransactions().stream().filter(LoanTransaction::isChargeback).toList();
Review Comment:
Same thing as above, usability perspective, let's move this into the Loan to
get only the chargeBack transactions. Even better if you make it more abstract
and do like this:
```
loan.getLoanTransactions(LoanTransaction::isChargeback);
```
##########
fineract-provider/src/main/java/org/apache/fineract/portfolio/delinquency/service/LoanDelinquencyDomainServiceImpl.java:
##########
@@ -66,10 +69,11 @@ public CollectionData getOverdueCollectionData(final Loan
loan) {
amountAvailable =
installment.getTotalPaid(loanCurrency).getAmount();
- for (LoanTransactionToRepaymentScheduleMapping
mappingInstallment : installment
-
.getLoanTransactionToRepaymentScheduleMappings()) {
- final LoanTransaction loanTransaction =
mappingInstallment.getLoanTransaction();
- if (loanTransaction.isChargeback()) {
+ boolean isLatestInstallment =
installment.equals(latestInstallment);
+ for (LoanTransaction loanTransaction :
chargebackTransactions) {
+ if
(!loanTransaction.getTransactionDate().isBefore(installment.getFromDate()) &&
((!isLatestInstallment
Review Comment:
We have many many places with logic like this and I always feel they are a
bit hard to read. Do you think it'd make sense to extract these individual
condition pieces into variables before the if statement? Like this:
```
boolean isTransactionBeforeInstallmentStart =
loanTransaction.getTransactionDate().isBefore(installment.getFromDate();
....
if (!isTransactionBeforeInstallmentStart && ...) {
...
}
```
Also, positive conditions are usually better, please evaluate whether we can
get rid of the negate operator and rename the variable accordingly.
##########
fineract-provider/src/main/java/org/apache/fineract/portfolio/delinquency/service/LoanDelinquencyDomainServiceImpl.java:
##########
@@ -66,10 +69,11 @@ public CollectionData getOverdueCollectionData(final Loan
loan) {
amountAvailable =
installment.getTotalPaid(loanCurrency).getAmount();
- for (LoanTransactionToRepaymentScheduleMapping
mappingInstallment : installment
-
.getLoanTransactionToRepaymentScheduleMappings()) {
- final LoanTransaction loanTransaction =
mappingInstallment.getLoanTransaction();
- if (loanTransaction.isChargeback()) {
+ boolean isLatestInstallment =
installment.equals(latestInstallment);
Review Comment:
Be careful with this. The LoanRepaymentScheduleInstallment doesn't have the
equals/hashCode overriden. The often given advice is to do equality checks
based on identity and override the equals/hashCode accordingly.
Can't we check the ID here instead of the full object?
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
