fineract-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From GitBox <...@apache.org>
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:21:40 GMT

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 too minor to
hold back merging this important fix, so I'll just go ahead and merge it ASAP, and let you
raise a very small minor follow-up PR, if you like (or not, fine). _I originally wrote this
before seeing the Spotless style failure, if you're going over this again, might as well do
that too here - if you agree that makes sense?_




----------------------------------------------------------------
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



Mime
View raw message