[GitHub] [fineract] vidakovic commented on a diff in pull request #2724: FINERACT-1761: New Charge Adjustment transaction

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

vidakovic commented on code in PR #2724:
URL: https://github.com/apache/fineract/pull/2724#discussion_r1017010859


##########
fineract-provider/src/main/java/org/apache/fineract/portfolio/loanaccount/service/LoanApplicationWritePlatformServiceJpaRepositoryImpl.java:
##########
@@ -1215,7 +1211,7 @@ private void logAsErrorUnexpectedDataIntegrityException(final Exception dve) {
     public CommandProcessingResult deleteApplication(final Long loanId) {
 
         final Loan loan = retrieveLoanBy(loanId);
-        checkClientOrGroupActive(loan);
+        loan.checkClientOrGroupActive();

Review Comment:
   I don't think it's not a recommendable strategy to move business rules into that entity class... Loan is already 7000 LOC long... having business rules embedded there leads to all kinds of weird situations... the worst: at some point you will need other services or components, but the JPA entities are not part of the whole Spring dependency injection lifecycle... which then leads to manually injecting "helpers" into the Loan class (this is already happening here)... 
   Also: moving business rules into that entity somehow assumes that they are set in stone. What do we do tomorrow if we want to add other rules for other scenarios? This is either going to end in a if-then-else fest (because it's not easy to reason about) or more functions added to this class... both not ideal, because 7K lines of code is just too much by all standards.
   What was wrong with leaving those functions with the service? Maybe I'm missing here something?



-- 
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: commits-unsubscribe@fineract.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org