patternjavaMinor
DAO Layer Code Review
Viewed 0 times
codedaolayerreview
Problem
Please review my following code for DAO layer.
```
public List getListOfChannels() throws DataAccessException {
// will have the channel in form of List
// to hold the channels list
List 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();
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) {
```
public List getListOfChannels() throws DataAccessException {
// will have the channel in form of List
// to hold the channels list
List 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();
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) {
Solution
There are a couple of improvements that can be made here:
because it make your code difficult to maintain
instead of array list. It has O(1) on insertion in worst case,
whereas array list has O(n)
block. You will lose all details of your original exception
operations with memory, because strings are immutable. use *.format
instead
- 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
Context
StackExchange Code Review Q#23609, answer score: 3
Revisions (0)
No revisions yet.