galovics commented on code in PR #2355:
URL: https://github.com/apache/fineract/pull/2355#discussion_r895933966
##########
fineract-provider/src/main/java/org/apache/fineract/accounting/journalentry/service/AccountingProcessorHelper.java:
##########
@@ -777,11 +777,11 @@ public void
createCashBasedJournalEntriesAndReversalsForSavingsCharges(final Off
}
public LoanTransaction getLoanTransactionById(final long
loanTransactionId) {
- return
this.loanTransactionRepository.findById(loanTransactionId).get();
+ return this.loanTransactionRepository.getById(loanTransactionId);
Review Comment:
findById and getById is fundamentally different.
The first one is returning an actual entity object if found while the other
one (getById) is returning only a reference to the object.
You can imagine the 2 by comparing:
- EntityManager#find
- EntityManager#getReference
The latter one is only a lazy reference to the object and will be loaded
upon access (at least for most JPA providers).
I'd say let's not mess around the references so to simply get rid of the
sonar issues, let's go with
`this.loanTransactionRepository.findById(loanTransactionId).orElse(null)`
##########
fineract-provider/src/main/java/org/apache/fineract/accounting/journalentry/service/AccountingProcessorHelper.java:
##########
@@ -777,11 +777,11 @@ public void
createCashBasedJournalEntriesAndReversalsForSavingsCharges(final Off
}
public LoanTransaction getLoanTransactionById(final long
loanTransactionId) {
- return
this.loanTransactionRepository.findById(loanTransactionId).get();
+ return this.loanTransactionRepository.getById(loanTransactionId);
}
public SavingsAccountTransaction getSavingsTransactionById(final long
savingsTransactionId) {
- return
this.savingsAccountTransactionRepository.findById(savingsTransactionId).get();
+ return
this.savingsAccountTransactionRepository.getById(savingsTransactionId);
Review Comment:
Same as above.
##########
fineract-provider/src/main/java/org/apache/fineract/notification/eventandlistener/NotificationEventListener.java:
##########
@@ -52,7 +52,7 @@ public void receive(NotificationData notificationData) {
if (notificationData.getOfficeId() != null) {
List<Long> tempUserIds = new ArrayList<>(userIds);
for (Long userId : tempUserIds) {
- AppUser appUser = appUserRepository.findById(userId).get();
+ AppUser appUser = appUserRepository.getById(userId);
Review Comment:
Same as above.
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/configuration/domain/ConfigurationDomainServiceJpa.java:
##########
@@ -115,13 +115,13 @@ public boolean isConstraintApproachEnabledForDatatables()
{
@Override
public boolean isEhcacheEnabled() {
- return this.cacheTypeRepository.findById(1L).get().isEhcacheEnabled();
+ return this.cacheTypeRepository.getById(1L).isEhcacheEnabled();
}
@Transactional
@Override
public void updateCache(final CacheType cacheType) {
- final PlatformCache cache =
this.cacheTypeRepository.findById(1L).get();
+ final PlatformCache cache = this.cacheTypeRepository.getById(1L);
Review Comment:
This should be something similar to this:
```
this.cacheTypeRepository.findById(1L).ifPresent(cache -> {
cache.update(cacheType);
this.cacheTypeRepository.save(cache);
}
```
##########
fineract-provider/src/main/java/org/apache/fineract/portfolio/account/service/StandingInstructionWritePlatformServiceImpl.java:
##########
@@ -183,7 +183,7 @@ public CommandProcessingResult update(final Long id, final
JsonCommand command)
@Override
public CommandProcessingResult delete(final Long id) {
- AccountTransferStandingInstruction standingInstructionsForUpdate =
this.standingInstructionRepository.findById(id).get();
+ AccountTransferStandingInstruction standingInstructionsForUpdate =
this.standingInstructionRepository.getById(id);
Review Comment:
Same as above.
##########
fineract-provider/src/main/java/org/apache/fineract/portfolio/savings/service/SavingsAccountWritePlatformServiceJpaRepositoryImpl.java:
##########
@@ -376,14 +376,14 @@ public CommandProcessingResult deposit(final Long
savingsId, final JsonCommand c
LOG.debug("Deposit account has been created: {} ", deposit);
- GroupSavingsIndividualMonitoring gsim =
gsimRepository.findById(account.getGsim().getId()).get();
+ GroupSavingsIndividualMonitoring gsim =
gsimRepository.getById(account.getGsim().getId());
LOG.info("parent deposit : {} ", gsim.getParentDeposit());
LOG.info("child account : {} ", savingsId);
BigDecimal currentBalance = gsim.getParentDeposit();
BigDecimal newBalance = currentBalance.add(transactionAmount);
gsim.setParentDeposit(newBalance);
gsimRepository.save(gsim);
- LOG.info("balance after making deposit : {} ",
gsimRepository.findById(account.getGsim().getId()).get().getParentDeposit());
+ LOG.info("balance after making deposit : {} ",
gsimRepository.getById(account.getGsim().getId()).getParentDeposit());
Review Comment:
This is anyway a bad pattern because we're making a database query just to
log something.
What if the logger is not even configured to info level but above, like
warn? Then the logger won't log anything but we still make the db call.
In those cases it's wise to wrap the log message into a level check like:
```
if (LOG.isInfoEnabled()) {
// do the expensive logging
}
```
##########
fineract-provider/src/main/java/org/apache/fineract/portfolio/savings/service/SavingsAccountWritePlatformServiceJpaRepositoryImpl.java:
##########
@@ -939,7 +939,7 @@ private void checkClientOrGroupActive(final SavingsAccount
account) {
public CommandProcessingResult bulkGSIMClose(final Long gsimId, final
JsonCommand command) {
final Long parentSavingId = gsimId;
- GroupSavingsIndividualMonitoring parentSavings =
gsimRepository.findById(parentSavingId).get();
+ GroupSavingsIndividualMonitoring parentSavings =
gsimRepository.getById(parentSavingId);
Review Comment:
Same as above.
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/creditbureau/service/CreditReportWritePlatformServiceImpl.java:
##########
@@ -146,20 +140,24 @@ public String addCreditReport(Long bureauId, File
creditReport, FormDataContentD
}
- private Optional<String> getCreditBureau(Long creditBureauID) {
+ private String getCreditBureau(Long creditBureauID) {
if (creditBureauID != null) {
Optional<CreditBureau> creditBureau =
this.creditBureauRepository.findById(creditBureauID);
if (creditBureau.isEmpty()) {
- return Optional.empty();
+
baseDataValidator.reset().failWithCode("creditBureau.has.not.been.Integrated");
Review Comment:
You could swap the condition and get rid of this exceptional case.
So it could be
```
if (!creditBureau.isEmpty()) {
return creditBureau.get().getName()
}
```
And the later exception throw logic will catch the error cases anyway.
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/configuration/domain/ConfigurationDomainServiceJpa.java:
##########
@@ -115,13 +115,13 @@ public boolean isConstraintApproachEnabledForDatatables()
{
@Override
public boolean isEhcacheEnabled() {
- return this.cacheTypeRepository.findById(1L).get().isEhcacheEnabled();
+ return this.cacheTypeRepository.getById(1L).isEhcacheEnabled();
Review Comment:
This could be like this:
`this.cacheTypeRepository.findById(1L).map(PlatformCache::isEhcacheEnabled).orElse(false)`
##########
fineract-provider/src/main/java/org/apache/fineract/notification/service/NotificationWritePlatformServiceImpl.java:
##########
@@ -83,7 +83,7 @@ public Long notify(Collection<Long> userIds, String
objectType, Long objectId, S
private List<Long> insertIntoNotificationMapper(Collection<Long> userIds,
Long generatedNotificationId) {
List<Long> mappedIds = new ArrayList<>();
for (Long userId : userIds) {
- AppUser appUser = this.appUserRepository.findById(userId).get();
+ AppUser appUser = this.appUserRepository.getById(userId);
Review Comment:
Same as above.
##########
fineract-provider/src/main/java/org/apache/fineract/accounting/journalentry/service/AccountingProcessorHelper.java:
##########
@@ -924,7 +924,7 @@ private void createDebitJournalEntryForSavings(final Office
office, final String
String modifiedTransactionId = transactionId;
if (StringUtils.isNumeric(transactionId)) {
long id = Long.parseLong(transactionId);
- savingsAccountTransaction =
this.savingsAccountTransactionRepository.findById(id).get();
+ savingsAccountTransaction =
this.savingsAccountTransactionRepository.getById(id);
Review Comment:
Same as above.
##########
fineract-provider/src/main/java/org/apache/fineract/accounting/provisioning/service/ProvisioningEntriesWritePlatformServiceJpaRepositoryImpl.java:
##########
@@ -210,11 +210,11 @@ private Collection<LoanProductProvisioningEntry>
generateLoanProvisioningEntry(P
.retrieveLoanProductsProvisioningData(date);
Map<Integer, LoanProductProvisioningEntry> provisioningEntries = new
HashMap<>();
for (LoanProductProvisioningEntryData data : entries) {
- LoanProduct loanProduct =
this.loanProductRepository.findById(data.getProductId()).get();
+ LoanProduct loanProduct =
this.loanProductRepository.getById(data.getProductId());
Office office =
this.officeRepositoryWrapper.findOneWithNotFoundDetection(data.getOfficeId());
- ProvisioningCategory provisioningCategory =
provisioningCategoryRepository.findById(data.getCategoryId()).get();
- GLAccount liabilityAccount =
glAccountRepository.findById(data.getLiablityAccount()).get();
- GLAccount expenseAccount =
glAccountRepository.findById(data.getExpenseAccount()).get();
+ ProvisioningCategory provisioningCategory =
provisioningCategoryRepository.getById(data.getCategoryId());
Review Comment:
Same as above.
##########
fineract-provider/src/main/java/org/apache/fineract/portfolio/loanaccount/service/LoanApplicationWritePlatformServiceJpaRepositoryImpl.java:
##########
@@ -1498,7 +1498,7 @@ public CommandProcessingResult
undoGLIMLoanApplicationApproval(final Long loanId
// GroupLoanIndividualMonitoringAccount
// glimAccount=glimRepository.findOne(loanId);
final Long parentLoanId = loanId;
- GroupLoanIndividualMonitoringAccount parentLoan =
glimRepository.findById(parentLoanId).get();
+ GroupLoanIndividualMonitoringAccount parentLoan =
glimRepository.getById(parentLoanId);
Review Comment:
Same as above.
##########
fineract-provider/src/main/java/org/apache/fineract/accounting/journalentry/service/AccountingProcessorHelper.java:
##########
@@ -833,7 +833,7 @@ private void createCreditJournalEntryForSavings(final
Office office, final Strin
String modifiedTransactionId = transactionId;
if (StringUtils.isNumeric(transactionId)) {
long id = Long.parseLong(transactionId);
- savingsAccountTransaction =
this.savingsAccountTransactionRepository.findById(id).get();
+ savingsAccountTransaction =
this.savingsAccountTransactionRepository.getById(id);
Review Comment:
Same as above.
##########
fineract-provider/src/main/java/org/apache/fineract/accounting/journalentry/service/AccountingProcessorHelper.java:
##########
@@ -904,7 +904,7 @@ private void createDebitJournalEntryForLoan(final Office
office, final String cu
String modifiedTransactionId = transactionId;
if (StringUtils.isNumeric(transactionId)) {
long id = Long.parseLong(transactionId);
- loanTransaction =
this.loanTransactionRepository.findById(id).get();
+ loanTransaction = this.loanTransactionRepository.getById(id);
Review Comment:
Same as above.
##########
fineract-provider/src/main/java/org/apache/fineract/portfolio/account/service/AccountTransfersWritePlatformServiceImpl.java:
##########
@@ -440,7 +440,7 @@ public Long transferFunds(final AccountTransferDTO
accountTransferDTO) {
// if the savings account is GSIM, update its parent as well
if (toSavingsAccount.getGsim() != null) {
- GroupSavingsIndividualMonitoring gsim =
gsimRepository.findById(toSavingsAccount.getGsim().getId()).get();
+ GroupSavingsIndividualMonitoring gsim =
gsimRepository.getById(toSavingsAccount.getGsim().getId());
Review Comment:
Same as above.
##########
fineract-provider/src/main/java/org/apache/fineract/accounting/provisioning/service/ProvisioningEntriesWritePlatformServiceJpaRepositoryImpl.java:
##########
@@ -210,11 +210,11 @@ private Collection<LoanProductProvisioningEntry>
generateLoanProvisioningEntry(P
.retrieveLoanProductsProvisioningData(date);
Map<Integer, LoanProductProvisioningEntry> provisioningEntries = new
HashMap<>();
for (LoanProductProvisioningEntryData data : entries) {
- LoanProduct loanProduct =
this.loanProductRepository.findById(data.getProductId()).get();
+ LoanProduct loanProduct =
this.loanProductRepository.getById(data.getProductId());
Review Comment:
Same as above.
##########
fineract-provider/src/main/java/org/apache/fineract/portfolio/loanaccount/service/LoanWritePlatformServiceJpaRepositoryImpl.java:
##########
@@ -341,7 +341,7 @@ private LoanLifecycleStateMachine
defaultLoanLifecycleStateMachine() {
@Override
public CommandProcessingResult disburseGLIMLoan(final Long loanId, final
JsonCommand command) {
final Long parentLoanId = loanId;
- GroupLoanIndividualMonitoringAccount parentLoan =
glimRepository.findById(parentLoanId).get();
+ GroupLoanIndividualMonitoringAccount parentLoan =
glimRepository.getById(parentLoanId);
Review Comment:
Same as above.
##########
fineract-provider/src/main/java/org/apache/fineract/accounting/journalentry/service/AccountingProcessorHelper.java:
##########
@@ -854,7 +854,7 @@ private void createCreditJournalEntryForLoan(final Office
office, final String c
String modifiedTransactionId = transactionId;
if (StringUtils.isNumeric(transactionId)) {
long id = Long.parseLong(transactionId);
- loanTransaction =
this.loanTransactionRepository.findById(id).get();
+ loanTransaction = this.loanTransactionRepository.getById(id);
Review Comment:
Same as above.
##########
fineract-provider/src/main/java/org/apache/fineract/portfolio/loanaccount/service/LoanApplicationWritePlatformServiceJpaRepositoryImpl.java:
##########
@@ -1577,7 +1577,7 @@ public CommandProcessingResult
rejectGLIMApplicationApproval(final Long glimId,
// GroupLoanIndividualMonitoringAccount
// glimAccount=glimRepository.findOne(loanId);
final Long parentLoanId = glimId;
- GroupLoanIndividualMonitoringAccount parentLoan =
glimRepository.findById(parentLoanId).get();
+ GroupLoanIndividualMonitoringAccount parentLoan =
glimRepository.getById(parentLoanId);
Review Comment:
Same as above.
##########
fineract-provider/src/main/java/org/apache/fineract/portfolio/loanaccount/service/LoanApplicationWritePlatformServiceJpaRepositoryImpl.java:
##########
@@ -1338,7 +1338,7 @@ public void validateMultiDisbursementData(final
JsonCommand command, LocalDate e
public CommandProcessingResult approveGLIMLoanAppication(final Long
loanId, final JsonCommand command) {
final Long parentLoanId = loanId;
- GroupLoanIndividualMonitoringAccount parentLoan =
glimRepository.findById(parentLoanId).get();
+ GroupLoanIndividualMonitoringAccount parentLoan =
glimRepository.getById(parentLoanId);
Review Comment:
Same as above.
##########
fineract-provider/src/main/java/org/apache/fineract/portfolio/loanaccount/service/LoanWritePlatformServiceJpaRepositoryImpl.java:
##########
@@ -938,7 +938,7 @@ public CommandProcessingResult makeGLIMLoanRepayment(final
Long loanId, final Js
final Long parentLoanId = loanId;
- glimRepository.findById(parentLoanId).get();
+ glimRepository.getById(parentLoanId);
Review Comment:
I think the intention here was to actually fail the execution in case the
parentLoanId is not found but with this refactoring we're losing that.
##########
fineract-provider/src/main/java/org/apache/fineract/portfolio/loanaccount/service/LoanWritePlatformServiceJpaRepositoryImpl.java:
##########
@@ -848,7 +848,7 @@ public CommandProcessingResult undoGLIMLoanDisbursal(final
Long loanId, final Js
// GroupLoanIndividualMonitoringAccount
// glimAccount=glimRepository.findOne(loanId);
final Long parentLoanId = loanId;
- GroupLoanIndividualMonitoringAccount parentLoan =
glimRepository.findById(parentLoanId).get();
+ GroupLoanIndividualMonitoringAccount parentLoan =
glimRepository.getById(parentLoanId);
Review Comment:
Same as above.
##########
fineract-provider/src/main/java/org/apache/fineract/portfolio/meeting/service/MeetingWritePlatformServiceJpaRepositoryImpl.java:
##########
@@ -164,7 +164,7 @@ private CalendarInstance getCalendarInstance(final
JsonCommand command) {
/*
* If group is within a center then center entityType should be
passed for retrieving CalendarInstance.
*/
- final Group group = this.groupRepository.findById(entityId).get();
+ final Group group = this.groupRepository.getById(entityId);
Review Comment:
Same as above.
##########
fineract-provider/src/main/java/org/apache/fineract/portfolio/savings/service/SavingsAccountWritePlatformServiceJpaRepositoryImpl.java:
##########
@@ -376,14 +376,14 @@ public CommandProcessingResult deposit(final Long
savingsId, final JsonCommand c
LOG.debug("Deposit account has been created: {} ", deposit);
- GroupSavingsIndividualMonitoring gsim =
gsimRepository.findById(account.getGsim().getId()).get();
+ GroupSavingsIndividualMonitoring gsim =
gsimRepository.getById(account.getGsim().getId());
Review Comment:
Same as above.
##########
fineract-provider/src/main/java/org/apache/fineract/portfolio/savings/domain/SavingsAccountAssembler.java:
##########
@@ -432,7 +432,7 @@ public SavingsAccount assembleFrom(final Client client,
final Group group, final
}
accountType = AccountType.JLG;
}
- final SavingsProduct product =
this.savingProductRepository.findById(productId).get();
+ final SavingsProduct product =
this.savingProductRepository.getById(productId);
Review Comment:
Same as above.
##########
fineract-provider/src/main/java/org/apache/fineract/portfolio/savings/service/SavingsAccountWritePlatformServiceJpaRepositoryImpl.java:
##########
@@ -376,14 +376,14 @@ public CommandProcessingResult deposit(final Long
savingsId, final JsonCommand c
LOG.debug("Deposit account has been created: {} ", deposit);
- GroupSavingsIndividualMonitoring gsim =
gsimRepository.findById(account.getGsim().getId()).get();
+ GroupSavingsIndividualMonitoring gsim =
gsimRepository.getById(account.getGsim().getId());
LOG.info("parent deposit : {} ", gsim.getParentDeposit());
LOG.info("child account : {} ", savingsId);
BigDecimal currentBalance = gsim.getParentDeposit();
BigDecimal newBalance = currentBalance.add(transactionAmount);
gsim.setParentDeposit(newBalance);
gsimRepository.save(gsim);
- LOG.info("balance after making deposit : {} ",
gsimRepository.findById(account.getGsim().getId()).get().getParentDeposit());
+ LOG.info("balance after making deposit : {} ",
gsimRepository.getById(account.getGsim().getId()).getParentDeposit());
Review Comment:
Same as above.
##########
fineract-provider/src/main/java/org/apache/fineract/portfolio/savings/service/SavingsAccountWritePlatformServiceJpaRepositoryImpl.java:
##########
@@ -448,7 +448,7 @@ public CommandProcessingResult withdrawal(final Long
savingsId, final JsonComman
transactionAmount, paymentDetail, transactionBooleanValues,
backdatedTxnsAllowedTill);
if (isGsim && (withdrawal.getId() != null)) {
- GroupSavingsIndividualMonitoring gsim =
gsimRepository.findById(account.getGsim().getId()).get();
+ GroupSavingsIndividualMonitoring gsim =
gsimRepository.getById(account.getGsim().getId());
Review Comment:
Same as above.
##########
fineract-provider/src/main/java/org/apache/fineract/portfolio/savings/service/SavingsAccountWritePlatformServiceJpaRepositoryImpl.java:
##########
@@ -225,7 +225,7 @@ public
SavingsAccountWritePlatformServiceJpaRepositoryImpl(final PlatformSecurit
public CommandProcessingResult gsimActivate(final Long gsimId, final
JsonCommand command) {
Long parentSavingId = gsimId;
- GroupSavingsIndividualMonitoring parentSavings =
gsimRepository.findById(parentSavingId).get();
+ GroupSavingsIndividualMonitoring parentSavings =
gsimRepository.getById(parentSavingId);
Review Comment:
Same as above.
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/creditbureau/service/CreditReportWritePlatformServiceImpl.java:
##########
@@ -146,20 +140,24 @@ public String addCreditReport(Long bureauId, File
creditReport, FormDataContentD
}
- private Optional<String> getCreditBureau(Long creditBureauID) {
+ private String getCreditBureau(Long creditBureauID) {
Review Comment:
Let's rename the method. It's not returning a CreditBureau object but only
its name. Should be `getCreditBureauName`
##########
fineract-provider/src/main/java/org/apache/fineract/portfolio/savings/service/SavingsApplicationProcessWritePlatformServiceJpaRepositoryImpl.java:
##########
@@ -612,7 +612,7 @@ public CommandProcessingResult
undoApplicationApproval(final Long savingsId, fin
public CommandProcessingResult rejectGSIMApplication(final Long gsimId,
final JsonCommand command) {
final Long parentSavingId = gsimId;
- GroupSavingsIndividualMonitoring parentSavings =
gsimRepository.findById(parentSavingId).get();
+ GroupSavingsIndividualMonitoring parentSavings =
gsimRepository.getById(parentSavingId);
Review Comment:
Same as above.
##########
fineract-provider/src/main/java/org/apache/fineract/portfolio/savings/service/SavingsApplicationProcessWritePlatformServiceJpaRepositoryImpl.java:
##########
@@ -552,7 +552,7 @@ public CommandProcessingResult approveApplication(final
Long savingsId, final Js
@Override
public CommandProcessingResult undoGSIMApplicationApproval(final Long
gsimId, final JsonCommand command) {
final Long parentSavingId = gsimId;
- GroupSavingsIndividualMonitoring parentSavings =
gsimRepository.findById(parentSavingId).get();
+ GroupSavingsIndividualMonitoring parentSavings =
gsimRepository.getById(parentSavingId);
Review Comment:
Same as above.
##########
fineract-provider/src/main/java/org/apache/fineract/portfolio/shareaccounts/service/ShareAccountSchedularServiceImpl.java:
##########
@@ -49,7 +49,7 @@ public ShareAccountSchedularServiceImpl(final
ShareAccountDividendRepository sha
@Transactional
public void postDividend(final Long dividendDetailId, final Long
savingsId) {
- ShareAccountDividendDetails shareAccountDividendDetails =
this.shareAccountDividendRepository.findById(dividendDetailId).get();
+ ShareAccountDividendDetails shareAccountDividendDetails =
this.shareAccountDividendRepository.getById(dividendDetailId);
Review Comment:
Same as above.
##########
fineract-provider/src/main/java/org/apache/fineract/portfolio/savings/service/SavingsApplicationProcessWritePlatformServiceJpaRepositoryImpl.java:
##########
@@ -488,7 +488,7 @@ public CommandProcessingResult approveGSIMApplication(final
Long gsimId, final J
// GroupLoanIndividualMonitoringAccount
// glimAccount=glimRepository.findOne(loanId);
Long parentSavingId = gsimId;
- GroupSavingsIndividualMonitoring parentSavings =
gsimRepository.findById(parentSavingId).get();
+ GroupSavingsIndividualMonitoring parentSavings =
gsimRepository.getById(parentSavingId);
Review Comment:
Same as above.
--
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]
