3
\$\begingroup\$

Please review my following code for DAO layer.

public List<Channel> getListOfChannels() throws DataAccessException {
        // will have the channel in form of List
        // to hold the channels list
        List<Channel> listChannels = null;
        Connection conn = null;
        Statement statement = null;
        ResultSet resultSet = null;

        try {
            // get the db connection from pool
                // this is DBCP lib on top doing this 
            conn = ManageConnections.getConnection();
            statement = conn.createStatement(ResultSet.TYPE_SCROLL_INSENSITIVE,
                    ResultSet.CONCUR_READ_ONLY);

            final String QUERY_STRING = "Select * from channel";
            resultSet = statement.executeQuery(QUERY_STRING);

            // is this good practice to put this ?
            if (isResultSetEmptyOrNull(resultSet)) {
                throw new DataAccessException(
                        "No more data of Channels found from db");
            }

            Channel channel = null;

            listChannels = new ArrayList<Channel>();

            while (resultSet.next()) {
                // get the object from the result set and 


                listChannels.add(channel);
            }

            log.debug("getListOfChannels Got the list of channels "
                    + listChannels);

                } catch (SQLException ex) {
            throw new DataAccessException(SQL_EXCEPTION + ex, ex);
        } catch (DBConnectionException ex) {
            // re thrown, no logging
            throw new DataAccessException(ex);
        } catch (Exception ex) {
            // Generic exception thrown , now throw Custom Exceptionn
            throw new DataAccessException(GENERIC_EXCEPTION + ex, ex);
        } finally {
            try {
                if (conn != null) {
                    // this is DBCP lib on top doing this 
                    ManageConnections.close(conn);
                }
                if (statement != null) {
                    statement.close();
                }
                if (resultSet != null) {
                    resultSet.close();
                }
            } catch (SQLException ex) {
                throw new DataAccessException(
                        "Exception while closing resource" + ex, ex);
            } catch (DBConnectionException ex) {
                throw new DataAccessException(
                        "Exception while closing resource" + ex, ex);
            }
        }

        return listChannels;
\$\endgroup\$
6
  • 1
    \$\begingroup\$ Even if this code was perfect, what is your plan for avoiding repeating it? \$\endgroup\$ Mar 8, 2013 at 11:12
  • \$\begingroup\$ Do you have Java 7? Then an ARM block would be helpful. \$\endgroup\$
    – Landei
    Mar 8, 2013 at 14:47
  • \$\begingroup\$ @abuzittingillifirca didnt get you ? \$\endgroup\$
    – ajduke
    Mar 9, 2013 at 18:16
  • \$\begingroup\$ @Landei Nope! we didnt migrated to Java7 yet. \$\endgroup\$
    – ajduke
    Mar 9, 2013 at 18:17
  • 1
    \$\begingroup\$ @ajduke Opening/closing resources (connections, transactions, statements, resultsets) and exception handling etc, which are almost all the code you've provided, are common to all data access methods; and somehow should be refactored out. All you should need to write in a dao implementation is something like em.createQuery("select c from Channel c").toResultList() \$\endgroup\$ Mar 11, 2013 at 7:24

1 Answer 1

3
\$\begingroup\$

There are a couple of improvements that can be made here:

  • a) your QUERY_STRING is magic string. Move it to a different place, because it make your code difficult to maintain
  • user linked list instead of array list. It has O(1) on insertion in worst case, whereas array list has O(n)
  • do not rethrow exceptions in finally block. You will lose all details of your original exception
  • every time when you concatenate strings with "+" you make too many operations with memory, because strings are immutable. use *.format instead
\$\endgroup\$
2
  • \$\begingroup\$ can you explain more on "why not to rethrow in finally block" ? \$\endgroup\$
    – ajduke
    Mar 9, 2013 at 18:19
  • 1
    \$\begingroup\$ Can you explain more on "why not to rethrow in finally block" ? Your finally will get executed no matter what. Any SQL Exception thrown in finally block will discard exceptions thrown before. \$\endgroup\$
    – PRSD
    Mar 12, 2013 at 21:47

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.