[GitHub] [fineract] ptuomola commented on a change in pull request #1184: FINERACT-1089 removing the rs.previous()

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

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