vidakovic commented on code in PR #2436:
URL: https://github.com/apache/fineract/pull/2436#discussion_r951225524
##########
fineract-provider/src/main/java/org/apache/fineract/portfolio/savings/domain/SavingsAccountTransactionRepository.java:
##########
@@ -49,4 +50,8 @@ List<SavingsAccountTransaction>
findTransactionRunningBalanceBeforePivotDate(@Pa
@Query("select sat from SavingsAccountTransaction sat where sat.refNo =
:refNo")
List<SavingsAccountTransaction> findAllTransactionByRefNo(@Param("refNo")
String refNo);
+
+ @Query("select sat from SavingsAccountTransaction sat where
sat.savingsAccount.id = :savingsId and sat.dateOf <= :transactionDate and
sat.reversed=false ORDER BY sat.dateOf DESC, sat.id DESC")
Review Comment:
To be consistent with other (query) methods rename this function to:
"findBySavingsAccountIdAndLessThanDateOfAndReversedIsFalse". Just to be clear:
in this case you need the @Query annotation (because of the nested
"sat.savingsAccount.id") and there is (I think) no way to express this query
with Spring Data's "function magic". But another thing I'd do here is to do
sorting via a function parameter... this is usually the first thing that
changes if someone wants to reuse this query... and if sorting is defined in
the annotation then it's set in stone. So instead I'd define a function (all
together):
```
@Query("select sat from SavingsAccountTransaction sat where
sat.savingsAccount.id = :savingsId and sat.dateOf <= :transactionDate and
sat.reversed=false")
findBySavingsAccountIdAndLessThanDateOfAndReversedIsFalse(@Param("savingsId")
Long savingsId, @Param("transactionDate") LocalDate transactionDate, Pageable
pageable, Sort sort)
```
Like this is more likely that this function gets reused instead of someone
else needing in another situation just a insignificant change (like sort order)
and creates then a similar function (if history is an indicator with yet
another naming scheme)... very hard for everyone else to wrap their heads
around. Of course this is not rocket science if we just look at this isolated
case... but if people read larger portions of code then the meandering naming
strategy we have is a bit of a problem.
Admittedly the function name is a bit long, but it's consistent with the
rest of the framework. I know that we have authors who introduced "retrieveXXX"
and similar prefixes, but I think we should go for consistency in naming.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
