fineract-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From GitBox <...@apache.org>
Subject [GitHub] [fineract] ptuomola commented on a change in pull request #1184: FINERACT-1089 removing the rs.previous()
Date Wed, 26 Aug 2020 02:57:50 GMT

ptuomola commented on a change in pull request #1184:
URL: https://github.com/apache/fineract/pull/1184#discussion_r477002949



##########
File path: fineract-provider/src/main/java/org/apache/fineract/portfolio/savings/service/DepositAccountInterestRateChartReadPlatformServiceImpl.java
##########
@@ -305,12 +305,8 @@ public DepositAccountInterestRateChartSlabData extractData(ResultSet
rs) throws
                     if (incentiveData != null) {
                         chartSlabData.addIncentives(incentiveData);
                     }
-                } else {
-                    rs.previous();

Review comment:
       @percyashu Yes as far as I can see, the easiest solution would be to have all the logic
advancing in the record set in the calling function - so not to move anywhere (forward or
backward) in the called functions. That would mean changing them to mapRow (as they are operating
on this one row only) rather than extractData (which operates on the whole resultset).
   
   But my problem in the previous comment is: I don't really understand why there's logic
to move forward / backward anyway? Typically that's there to e.g. combine multiple rows into
one. But in this case it returns just a single row and seems to ignore all the others? Is
this just to ensure the last record is returned or what's the intention? 
   
   So my suggestion would be to set up a test that checks the results being returned (and
covers this scenario where rs.next()/rs.previous() is triggered) - and then refactor the method,
with the test ensuring that the result remains the same. Of course that assumes the current
logic is actually correct... which would be great for someone with more functional knowledge
of the data model to confirm!




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