[GitHub] [fineract] vorburger commented on a change in pull request #1024: FINERACT-995: Rewriting logic to remove call to rs.previous()

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

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



##########
File path: fineract-provider/src/main/java/org/apache/fineract/portfolio/loanaccount/service/LoanArrearsAgingServiceImpl.java
##########
@@ -423,18 +423,14 @@ public OriginalScheduleExtractor(final String loanIdsAsString) {
 
             while (rs.next()) {
                 Long loanId = rs.getLong("loanId");
-                List<LoanSchedulePeriodData> periodDatas = new ArrayList<>();
-                LoanSchedulePeriodData loanSchedulePeriodData = fetchLoanSchedulePeriodData(rs);
-                periodDatas.add(loanSchedulePeriodData);
-                while (rs.next()) {
-                    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:
users@infra.apache.org