Multiple JDBC Statements in Single Try Catch Block. is it Good Practice?

Question

I am new to Java, and my Work is all related to JDBC - about inserting and processing data. Over all its working fine.

To reduce code am using single try{} catch()block to write multiple JDBC Statements and Prepared Statements.

Example Code:

public void dashboardReports()
{
    try {

        String total_stock_value="select sum(price*closingstock)as tsv from purchase_table";
        Statement ps_tsv=connection.createStatement();
        ResultSet set_tsv=ps_tsv.executeQuery(total_stock_value);
        if(set_tsv.next())
        {
            total_stock.setText(set_tsv.getString("tsv"));              
        }           

        String tota_sales="select sum(INVOICE_VALUE) as iv from  PARTYWISE_ACCOUNTS_LEDGER";
        Statement st_total_sales=connection.createStatement();
        ResultSet set_total_sales=st_total_sales.executeQuery(tota_sales);
        if(set_total_sales.next())
        {
            total_sales.setText(set_total_sales.getString("iv"));
        }           

        String total_purchases="select sum(CP_INVOICEVALUE)as cpi from COMPANY_PAYMENTS";
        Statement st_tps=connection.createStatement();
        ResultSet set_tps=st_tps.executeQuery(total_purchases);
        if(set_tps.next())
        {
            total_purchases_label.setText(set_tps.getString("cpi"));
        }

        String total_collectionss="select sum(PAYMENT_REC) as payrec from PARTYWISE_ACCOUNTS_LEDGER";
        Statement ps_toco=connection.createStatement();
        ResultSet set_toco=ps_toco.executeQuery(total_collectionss);
        if(set_toco.next())
        {
            total_collections.setText(set_toco.getString("payrec"));
        }

        String total_payments="select sum(CP_PAYMENTREC) as paid from COMPANY_PAYMENTS";
        Statement ps_topa=connection.createStatement();
        ResultSet set_topa=ps_topa.executeQuery(total_payments);
        if(set_topa.next())
        {
            total_payments_label.setText(set_topa.getString("paid"));
        }

    } catch (Exception e) {
        // TODO: handle except
    }
}

So is this Good Way to Handle or another other way?

As of now my code is working very fine, do we have any future problems with this kind of approach.

If this code works fine, you should submit it on our [Code Review](https://codereview.stackexchange.com/) sister site. — Joe C, Apr 25 '17 at 06:28
The biggest problem I see is that you don't close your resources, if you'd use try-with-resources that would be less of a problem, and you would automatically use smaller scoped try-blocks. BTW: Your code is inefficient: you are querying 2 tables 2 times, when you could have queried those two only once. — Mark Rotteveel, Apr 25 '17 at 06:50

GhostCat · Answer 1 · 2017-04-25T06:59:21.980

4

This violates the Single Responsbility and the Single Layer of Abstraction principles.

So although this code is technically valid; you should not only focus on its correctness, but also on its readability. And testability. And I think neither of does are "great" in the input you are showing.

Thus; coming from a clean code (quality) perspective; I would rather advise to go for something along the lines of:

outer method ...
  try {
    helperMethod1();
    helperMethod2();
  } catch( ...

with a small helper for each of the different cases you got there. And of course, you wouldn't stop there; but try to isolate common aspects of those helpers; and to maybe find ways to go with a single, more generic helper.

And of course: you try to avoid catching Exception if possible. Instead, you catch the most specific exception possible!

edited Apr 25 '17 at 06:59

answered Apr 25 '17 at 06:45

GhostCat

137,827
25
176
248

2

+1 I agree with what you are saying and would even like to add that as `helperMethod1` and `helperMethod2` are almost identical in functionality they could maybe be refactored to a single parameterized method – Scary Wombat Apr 25 '17 at 06:57
Great idea. Factored that into my answer. – GhostCat Apr 25 '17 at 06:59
@GhostCat: How to ensure clean coding in case of nested try blocks for transactions from this link - https://stackoverflow.com/a/30116055/4358787 – Gaurav Nov 21 '18 at 12:40
1

@gaurav Clean code says: single layer of abstracting. In clean code, there is no nesting. If at all, you have method A, which calls method B in a try block, and B calls C, in another try block. – GhostCat Nov 21 '18 at 13:01

score 2 · Answer 2 · answered Apr 25 '17 at 06:32

Since you are only doing SELECT operations here, there is no real need for an explicit transaction, because you are not changing the state of the database, and there is nothing to rollback. There is nothing wrong with grouping all SELECT statements inside a single try block. However, there is a potential drawback, namely that if one SELECT fails, your code will exit that try block and all subsequent queries will not run. If you can tolerate this, then you can leave your as is. An analogy to this would be a series of lightbulbs connected in serial; if one breaks, then they all go out.

An alternative to what you have would be to use separate try blocks for each query. Then, even if an exception were to happen in one of them, it is possible that the others could complete successfully. The analogy here would be a series of lightbulbs in a parallel circuit.

score 2 · Answer 3 · answered Apr 25 '17 at 06:39

2

I think your code is ok. You need close resultset, statement and connection in block finally.

answered Apr 25 '17 at 06:39

Don

61
2

score 1 · Answer 4 · answered Apr 25 '17 at 06:38

1

If you are happy with all subsequent SELECTs failing if one fails, then I would change the method to throw an exception

public void dashboardReports() throws SQLException 
{
 ....
}

and then catch the SQLException from the calling method.

Note I think it is better to throw/catch a SQLException rather than a Exception

answered Apr 25 '17 at 06:38

Scary Wombat

44,617
6
35
64

A bit similar to what I just wrote ... feedback is welcome there. – GhostCat Apr 25 '17 at 06:47

score 0 · Answer 5 · answered Apr 25 '17 at 06:33

Just make sure you close your statements and resultsets:

try {

    String total_stock_value="select sum(price*closingstock)as tsv from purchase_table";
    try (Statement ps_tsv=connection.createStatement();
        ResultSet set_tsv=ps_tsv.executeQuery(total_stock_value)) {
        if(set_tsv.next())
        {
            total_stock.setText(set_tsv.getString("tsv"));
        }
    }

    String tota_sales="select sum(INVOICE_VALUE) as iv from  PARTYWISE_ACCOUNTS_LEDGER";
    try (Statement st_total_sales=connection.createStatement();
            ResultSet set_total_sales=st_total_sales.executeQuery(tota_sales)) {
        if(set_total_sales.next())
        {
            total_sales.setText(set_total_sales.getString("iv"));
        }
    }

    String total_purchases="select sum(CP_INVOICEVALUE)as cpi from COMPANY_PAYMENTS";
    try (Statement st_tps=connection.createStatement();
            ResultSet set_tps=st_tps.executeQuery(total_purchases)) {
        if(set_tps.next())
        {
            total_purchases_label.setText(set_tps.getString("cpi"));
        }
    }

    String total_collectionss="select sum(PAYMENT_REC) as payrec from PARTYWISE_ACCOUNTS_LEDGER";
    try (Statement ps_toco=connection.createStatement();
            ResultSet set_toco=ps_toco.executeQuery(total_collectionss)) {
        if(set_toco.next())
        {
            total_collections.setText(set_toco.getString("payrec"));
        }
    }

    String total_payments="select sum(CP_PAYMENTREC) as paid from COMPANY_PAYMENTS";
    try (Statement ps_topa=connection.createStatement();
            ResultSet set_topa=ps_topa.executeQuery(total_payments)) {
        if(set_topa.next())
        {
            total_payments_label.setText(set_topa.getString("paid"));
        }
    }

} catch (Exception e) {
    // TODO: handle except
}

}

score 0 · Answer 6 · answered Apr 25 '17 at 06:42

1.You can have a method like , executeQuery(Connection conn, Statement st, String sql) to encapsulate and reduce your lines of code.

2.Don't rely on generic Exception , catch sql specific exception classes too

3.I don't see a finally block there to properly close resources unless you are doing that somewhere else. Alternatively, you can try using try with resource syntax to eliminate need of finally block

4.See what you need to do in catch block - do you need to propagate exception up above the chain or fail the program right there?

5.In my opinion, a ResultSet and Statement needs to live as short as they can so try closing them as soon as you can - don't wait to close them all in single chunk. Point#1 will help in achieving this.

From technical correctness and validity perspective, there is no harm in writing code the way you did- using a single try-catch for all SQLs and eating out any exceptions ( since I see only SELECT sqls there) but there are clean,readable and maintainable code and on that front, your code looks poor.

Vasu · Answer 7 · 2017-04-25T07:09:47.437

Though your code works, I strongly suggest to refactor it for better maintenance and readability as shown below. Also, ensure that the resources are closed properly:

public void dashboardReports() {
     handleTotalStocks();
     handleTotalSales();
     handleTotalPurchages();
     //Add others
}

handleTotalStocks() method:

private void handleTotalStocks() {
       String total_stock_value="select sum(price*closingstock)as tsv 
                     from purchase_table";

     try(Statement ps_tsv=connection.createStatement();
        ResultSet set_tsv=ps_tsv.executeQuery(total_stock_value);) {
         if(set_tsv.next()) {
            total_stock.setText(set_tsv.getString("tsv"));
         }
       }
  }
 //add other methods

score -1 · Answer 8 · answered Apr 25 '17 at 06:39

Better way is to create method that does common operations:

public String execute(String query) throws SQLException {
Statement ps_toco=connection.createStatement();
        ResultSet set_toco=ps_toco.executeQuery(query);
        return set_toco.next();
}

When you call this method surround it with try catch block.

Multiple JDBC Statements in Single Try Catch Block. is it Good Practice?

8 Answers8