fineract-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From GitBox <>
Subject [GitHub] [fineract] vorburger commented on a change in pull request #1024: FINERACT-995: Rewriting logic to remove call to rs.previous()
Date Sat, 20 Jun 2020 10:17:34 GMT

vorburger commented on a change in pull request #1024:

File path: fineract-provider/src/main/java/org/apache/fineract/portfolio/loanaccount/service/
@@ -423,18 +423,14 @@ public OriginalScheduleExtractor(final String loanIdsAsString) {
             while ( {
                 Long loanId = rs.getLong("loanId");
-                List<LoanSchedulePeriodData> periodDatas = new ArrayList<>();
-                LoanSchedulePeriodData loanSchedulePeriodData = fetchLoanSchedulePeriodData(rs);
-                periodDatas.add(loanSchedulePeriodData);
-                while ( {
-                    Long tempLoanId = rs.getLong("loanId");
-                    if (loanId.equals(tempLoanId)) {

Review comment:
       @ptuomola sorry for delay in reviewing this (I wanted to do it with a clear mind instead
of clicking through it half asleep a night after day job work...) and after staring at this
a little bit, I get it now - this makes good sense to me as well, and I have no objection
to merging this even without having any way to functionally test this myself either (no idea
if existing ITs, recently re-enable, would catch this, but doesn't matter, it "makes sense").

   One very small feedback I would have is that, now that I understand this logic, it seems
to me that moving that `scheduleDate.put(loanId, periodDatas);` now in line 449 (originally
434) up into the `if(periodDatas == null)` right after the `periodDatas = new ArrayList<>();`
would make it a tiny bit more clear and explicit what we're doing here? It's two minor to
hold back merging this important fix, so I'll just go ahead and merged, and let you raise
a very small minor follow-up PR, if you like (or not, fine).

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:

View raw message