Migration of Data from Shared database and acknowledged the same

Question

I wrote a method in java which fetches the data from third part database, store the results in our database and then acknowledged them by updating their tuples. I want you people to review my code, because it contains multiple try and catch and suggest me the best practice to improvise the same, Find the below java

public boolean pickSalayData(String yearMonth, String regionId, String circleId, Userdetail loginUser) throws MyExceptionHandler {
        String tableSuffix = yearMonth.substring(4, 6) + yearMonth.substring(0, 4);

        log.info("Pick Salary Data From ERP " + DateUtility.dateToStringDDMMMYYYY(new Date()));
        List<SalaryDetailReport> detailReports = hRMSPickSalaryDataDAO.findAll(yearMonth, regionId, circleId);

        TransactionDefinition def = new DefaultTransactionDefinition();
        TransactionStatus trstatus = transactionManager.getTransaction(def);
        try {
            List<SalaryDetailReport> salaryDetailReport = null;
            int countDetail = 0;
            if (detailReports != null && detailReports.size() > 0) {

                for (SalaryDetailReport salary : detailReports) {
                    try {
                        if (countDetail % COMMIT_COUNT == 0) {
                            if (salaryDetailReport != null) {
                                salaryDetailReportDAO.save(salaryDetailReport, tableSuffix);
                                reportHistoryDAO.save(salaryDetailReport, loginUser);
                            }
                            salaryDetailReport = new ArrayList<SalaryDetailReport>();
                        }
                        salaryDetailReport.add(salary);
                        countDetail++;

                    } catch (Exception e) {

                        log.error("Error on Save Salary Pay Head Details Data from ERP to Prayas .");
                        return false;
                    }
                }
                if (salaryDetailReport != null && salaryDetailReport.size() > 0) {
                    salaryDetailReportDAO.save(salaryDetailReport, tableSuffix);
                    reportHistoryDAO.save(salaryDetailReport, loginUser);
                }
            } else {
                throw new MyExceptionHandler("No record for Save in Database from ERP.");

            }
            salaryDetailReportDAO.update(tableSuffix, regionId, circleId);
            List<SalaryDetailReport> reports = salaryDetailReportDAO.findAll(tableSuffix, regionId, circleId);
            if (reports != null && reports.size() > 0) {
                for (SalaryDetailReport salaryDetail : reports) {
                    try {
                        SalaryDetail sd = new SalaryDetail();
                        sd.setDetailReport(salaryDetail);
                        salaryDetailDAO.save(sd, tableSuffix);
                    } catch (Exception e) {
                        log.error("Error occured", e);
                        e.printStackTrace();
                        throw new MyExceptionHandler(" Error :" + e.getMessage());
                    }
                }
                System.out.println("data found");

            } else {
                log.error("Salary Record Not Found.");
                throw new MyExceptionHandler("No record Found.");
            }
            salaryDetailDAO.updateEarningDeduction(tableSuffix);
            //salaryDetailDAO.updateEarningDeductionsInSDT();
            transactionManager.commit(trstatus);
            try {
                hRMSPickSalaryDataDAO.update(regionId, circleId, yearMonth);
                return true;
            } catch (Exception ex) {
                log.error("Error Occured while updating XXMPCD_SALARY_DETAIL_TABLE  : ", ex);
                return false;
            }

//            //   System.out.println("Completed =============================");
        } catch (MyExceptionHandler ex) {
            transactionManager.rollback(trstatus);
            ex.printStackTrace();
            log.error("Failed to Save Salary data  :" + ex.getMessage());
            return false;

        } catch (Exception ex) {
            transactionManager.rollback(trstatus);
            ex.printStackTrace();
            log.error("Error occured on Save Salary data.", ex);
            return false;
        }
    }

Answer 1

General

You misspelled pickSalaryData. That’s a poor start.

I will bet a day’s pay that none of your findAll methods will ever return null. Assuming that’s the case, don’t check for null. It’s confusing and makes the code harder to read. And if they can remove null, then they shouldn’t.

Don’t use null values when you can use empty collections and loop zero times.

Don’t check if something is size zero to skip a loop that wouldn’t do anything if the size is zero.

Minimize the amount of code inside a try-catch block where possible.

Don’t write code to look like a giant > sign. Use guard clauses. Instead of

if (condition) {
    // lots of nested code
} else {
    throw new MyException();
}

prefer

if (!condition) {
    throw new MyException();
}
// lots of code

!isEmpty is easer to read than size() > 0.

Your logic for saving salary reports is very messy and can be cleaned up. It can definitely be broken out into a method. Likewise for saving salary details.

It’s unclear why you need to manually batch your writes. Any reasonable ORM should handle that for you. I’ll assume it’s necessary for some reason, but it really shouldn’t be.

Your variable names are all confusing. COMMIT_COUNT should be BATCH_SIZE. salaryDetailReport might be more clearly named reportsToSave. salaryDetail should be salaryDetailReport. All of your names are of a different type than the name they have. It’s very important that this gets fixed.

Adding a SalaryDetail constructor that takes a SalaryDetailReport might be nice.

If you have a Logger, don’t write to System.out.

The Logger will print the stack trace correctly if you pass the exception in. You don’ t need to print it separately.

Exception Handling

MyExceptionHandler is something that handles exceptions. You have a MyException.

You’re not handling the expected Exception when doing the final save of the salary detail report.

Don’t catch Exception when you can catch a more specific exception type. Don’ throw Exception when you can throw a more specific exception type.

When logging an Exception always include the stack trace.

You’re not stopping the transaction if saving the salary report details fails, or if the pick salary data call fails.

When there’s an underlying exception, it’s nice to include it when constructing MyException.

You’re throwing, catching, and creating a lot of exceptions. By the contract, sometimes pickSalaryData fails with an exception, and sometimes it returns false. In practice, though, all exceptions are handled and the code returns true or false. Take the throws out of the method declaration.

Several of your catch blocks are just to log a message. Make sure you really need those messages to help identify where a problem is. If not, you can remove the catch block and just let the exception percolate up.

There’s no functional difference in your code between a MyException and an Exception. It would be best if you could make everything a MyException and deal with that. If not, you can at least take advantage of Java’s ability to handle multiple exception types in a single catch clause.

If you were to make all the modifications I suggest, your code might look something like:

public boolean pickSalaryData(final String yearMonth, final String regionId, final String circleId, final Userdetail loginUser) {
    final String tableSuffix = yearMonth.substring(4, 6) + yearMonth.substring(0, 4);

    this.log.info("Pick Salary Data From ERP " + DateUtility.dateToStringDDMMMYYYY(new Date()));
    final List<SalaryDetailReport> detailReports = this.hRMSPickSalaryDataDAO.findAll(yearMonth, regionId, circleId);

    final TransactionDefinition transactionDefinition = new DefaultTransactionDefinition();
    final TransactionStatus transaction = this.transactionManager.getTransaction(transactionDefinition);
    try {
        if (detailReports.isEmpty()) {
            throw new MyException("No record for Save in Database from ERP.");
        }

        this.saveSalaryReports(detailReports, tableSuffix, loginUser);
        this.salaryDetailReportDAO.update(tableSuffix, regionId, circleId);

        this.saveSalaryDetails(tableSuffix, regionId, circleId);
        this.salaryDetailDAO.updateEarningDeduction(tableSuffix);

        this.transactionManager.commit(transaction);

        try {
            this.hRMSPickSalaryDataDAO.update(regionId, circleId, yearMonth);
        } catch (final Exception e) {
            this.log.error("Error Occured while updating XXMPCD_SALARY_DETAIL_TABLE  : ", e);
            throw e;
        }

        return true;

    } catch (final Exception e) {
        this.transactionManager.rollback(transaction);
        this.log.error("Failed to Save Salary data  :" + e.getMessage(), e);
        return false;
    }
}

private void saveSalaryReports(
        final List<SalaryDetailReport> detailReports,
        final String tableSuffix,
        final Userdetail loginUser)
                throws Exception {

    final List<SalaryDetailReport> reportsToSave = new ArrayList<>(BATCH_SIZE);
    for (final SalaryDetailReport detailReport : detailReports) {
        reportsToSave.add(detailReport);
        if (reportsToSave.size() % BATCH_SIZE == 0) {
            this.saveReports(reportsToSave, tableSuffix, loginUser);
            reportsToSave.clear();
        }
    }
    this.saveReports(reportsToSave, tableSuffix, loginUser);
}

private void saveReports(final List<SalaryDetailReport> reportsToCommit, final String tableSuffix, final Userdetail loginUser) {
    try {
        this.salaryDetailReportDAO.save(reportsToCommit, tableSuffix);
        this.reportHistoryDAO.save(reportsToCommit, loginUser);
    } catch (final Exception e) {
        this.log.error("Error on Save Salary Pay Head Details Data from ERP to Prayas .", e);
        throw e;
    }
}

private void saveSalaryDetails(final String tableSuffix, final String regionId, final String circleId)
        throws MyException {

    final List<SalaryDetailReport> reports = this.salaryDetailReportDAO.findAll(tableSuffix, regionId, circleId);
    if (reports.isEmpty()) {
        this.log.error("Salary Record Not Found.");
        throw new MyException("No record Found.");
    }
    this.log.debug("data found");

    for (final SalaryDetailReport salaryDetail : reports) {
        this.salaryDetailDAO.save(new SalaryDetail(salaryDetail), tableSuffix);
    }
}