Basic CRUD DAO using JDBC to access my database

Question

I have a basic CRUD DAO using JDBC to access my database. I using a connection pool to get a connection in each method and then execute my commands. In some scenarios for update User, there might be cases where I don't have to update the whole DTO to the database. In my insert and my findbykey() methods, I found out quickly how complicated it gets. I'm thinking either my design of my DTO and DAO are poorly constructed or my logic for my code is poor.

Any help in either readability or redesign?

public class SQLUserDAO implements GenericDAO<User, String, Boolean>
{
 @Override
 public void update(User user, Boolean active) throws NotFoundException
 {
 // Create the ConnectionPool:
 JDBCConnectionPool pool = JDBCConnectionPool.getPoolInstance();
 // Get a connection:
 Connection con = pool.checkOut();
 // Return the connection:
 pool.checkIn(con);
 }
 @Override
 public void delete(User user, Boolean active) throws NotFoundException
 {
 // Create the ConnectionPool:
 JDBCConnectionPool pool = JDBCConnectionPool.getPoolInstance();
 // Get a connection:
 Connection con = pool.checkOut();
 // Return the connection:
 pool.checkIn(con);
 }
 @Override
 public User findByKey(String key, Boolean active) throws NotFoundException
 {
 DefaultUser tempUser = null;
 // Create the ConnectionPool:
 JDBCConnectionPool pool = JDBCConnectionPool.getPoolInstance();
 // Get a connection:
 Connection con = pool.checkOut();
 String query = "SELECT * FROM users WHERE user_name ='" + key + "'";
 try
 {
 PreparedStatement stmt = con.prepareStatement(query);
 ResultSet rs = stmt.executeQuery();
 String user_name = null, user_id = null, user_password = null, user_type = null, first_name = null, last_name = null, creation_date = null;
 while (rs.next())
 {
 user_name = (rs.getString(1));
 user_id = rs.getString(2);
 user_password = (rs.getString(3));
 user_type = (rs.getString(4));
 first_name = (rs.getString(5));
 last_name = (rs.getString(6));
 creation_date = (rs.getString(7));
 }
 /*
 * LOG.CONSOLE.debug(user_name); LOG.CONSOLE.debug(user_id);
 * LOG.CONSOLE.debug(user_password);
 * LOG.CONSOLE.debug(user_type);
 * LOG.CONSOLE.debug(first_name);
 * LOG.CONSOLE.debug(last_name);
 * LOG.CONSOLE.debug(creation_date);
 */
 String[] userTypeString = user_type.split("\\+");
 List<UserType> userType = new ArrayList<UserType>();
 if (userTypeString != null)
 {
 for (String useType : userTypeString)
 {
 char[] userTypeCharArray = useType
 .toCharArray();
 switch (userTypeCharArray[0])
 {
 case 'A' :
 userType.add(UserType.ADMIN);
 break;
 case 'S' :
 userType.add(UserType.SHAREHOLDER);
 break;
 case 'B' :
 userType.add(UserType.BROKER);
 break;
 }
 }
 }
 Date date = new SimpleDateFormat("dy mon d hh24:mi:ss yyyy")
 .parse(creation_date);
 tempUser = new DefaultUser(user_name,
 UUID.fromString(user_id), user_password, userType,
 first_name, last_name, date);
 }
 catch (SQLException e)
 {
 pool.checkIn(con);
 e.printStackTrace();
 }
 catch (ParseException e)
 {
 // TODO Auto-generated catch block
 e.printStackTrace();
 }
 pool.checkIn(con);
 return tempUser;
 }
 @Override
 public User findByValue(User object, Boolean active)
 throws NotFoundException
 {
 // Create the ConnectionPool:
 JDBCConnectionPool pool = JDBCConnectionPool.getPoolInstance();
 // Get a connection:
 Connection con = pool.checkOut();
 // Return the connection:
 pool.checkIn(con);
 return null;
 }
 @Override
 public void insert(User user, Boolean active) throws NotFoundException
 {
 // Create the ConnectionPool:
 JDBCConnectionPool pool = JDBCConnectionPool.getPoolInstance();
 // Get a connection:
 Connection conn = pool.checkOut();
 try
 {
 PreparedStatement preparedStatement;
 preparedStatement = pool
 .checkOut()
 .prepareStatement(
 "INSERT INTO users(user_name,user_id,user_password,creation_date,first_name,last_name,user_type)"
 + "VALUES (?,?,?,?,?,?,?)");
 preparedStatement.setString(1, user.getUserName());
 preparedStatement.setString(2, user.getUserId().toString());
 preparedStatement.setString(3, user.getClearTextPassword());
 preparedStatement.setString(4, user.getCreationDate()
 .toString());
 preparedStatement.setString(5, user.getFirstName());
 preparedStatement.setString(6, user.getLastName());
 StringBuilder userTypeCode = new StringBuilder();
 if (user.getUserTypes() != null)
 {
 for (UserType useType : user.getUserTypes())
 {
 switch (useType)
 {
 case ADMIN :
 userTypeCode.append(UserType.ADMIN
 .toString());
 break;
 case SHAREHOLDER :
 userTypeCode
 .append(UserType.SHAREHOLDER
 .toString());
 break;
 case BROKER :
 userTypeCode.append(UserType.BROKER
 .toString());
 break;
 }
 userTypeCode.append("+");
 }
 }
 preparedStatement.setString(7, userTypeCode.toString());
 preparedStatement.executeUpdate();
 }
 catch (SQLException e)
 {
 pool.checkIn(conn);
 e.printStackTrace();
 }
 // Return the connection:
 pool.checkIn(conn);
 }
 }

Answer 1

1. Use a JBDC connection pooling library

   Based on my rusty knowledge of the usage of common JDBC connection pools out there, your custom JDBCConnectionPool.getPoolInstance().checkOut().checkIn(connection) does smell of boilerplate template. A good pool manager should manage on the Connection object internally, so you usually just call connection.close() without having to explicitly worry what the pool manager is going to do about it. Also, you usually only need the reference to the pool manager once, so you can do that in the DAO's constructor instead of within each method.

2. Consider using parameters for your SELECT query too

   String query = "SELECT * FROM users WHERE user_name ='" + key + "'";
   

   This is ripe for SQL injection attacks, so let the underlying database driver handle parameters for you directly! Furthermore, comparing your SELECT and INSERT statements, it looks like the positioning of your INSERT prepared statement is different from the actual table schema returned by SELECT. This makes the code look slightly confusing initially: the user type is SELECTed via rs.getString(4) but stored as preparedStatement.setString(7, userTypeCode.toString()). Standardization will help eliminate these kinds of false bugs. This has a decent example of using PreparedStatement with a SELECT query.

3. Reduce your try-catch scope

   Somewhat related to the next point, but having a shorter try-catch scope makes it clearer what methods can throw what kinds of checked Exceptions, which will facilitate understanding.

   Not recommended (roughly putting it in your context):

   try {
    ResultSet resultSet = statement.executeQuery();
    // do a bunch of other non-SQL-related stuff that may throw ParseException
   } catch (SQLException e) {
    // ...
   } catch (ParseException e) {
    // ...
   }
   

   Recommended:

   try {
    ResultSet resultSet = statement.executeQuery();
    // do a bunch of other SQL-related stuff
   } catch (SQLException e) {
    // ...
   }
   // do a bunch of other non-SQL-related stuff that does not throw ParseException
   try {
    // do a bunch of other non-SQL-related stuff that may throw ParseException
   } catch (ParseException e) {
    // ...
   }
   // do a bunch of other non-SQL-related stuff that does not throw ParseException
   

4. Consider encapsulating the logic for mapping a new User or UserType in its own method/class

   char[] userTypeCharArray = useType.toCharArray();
   

   I suppose you have this line/chunk of code because you're still on Java 6, since Java 7 can do switch on String literals. Therefore, at the very least, you can consider putting this inside a method e.g. List<UserType> getUserTypes(String userTypeDbValue) so that the refactoring can be easily done in the future, when you migrate to newer Java versions. In fact, you should go further by having a separate class that can 'map' a ResultSet into your User object, and the logic to convert e.g. 'A+B+S' into a List of UserType.ADMIN, UserType.BROKER, UserType.SHAREHOLDER will belong there too.

5. Don't store password in plain-text :p

   Can't help but notice this in your insert() method: preparedStatement.setString(3, user.getClearTextPassword());. I hope this is only for an academic exercise, not something to be implemented in a public-facing Fortune-500 company's web portal. :p

6. User types in another table? (database design)

   (suggested by @Vogel612)

   This is not about your code, but the database design. Perhaps the reason you are storing user types as a column in the users table is because it works well for your existing use cases. Still, you may want to re-evaluate your database design to see if it can/should be in its own table, so that you can use foreign-key relationships in your favorite Relational Database Management System to represent their associations with users as such.

• Vogel612: keep in mind, that resultSet will not be available outside of the try-block scope like that. Additionally the UserType thing should be a completely separate table, but I can't answer right now, feel free to include it ;)
• h.j.k.: @Vogel612 added, thanks for the suggestion. Also clarified the point about resultSet... can't think of a simple way to exemplify what I meant without spilling fully into my next point. Hopefully the current edit is clear enough.
• user3590149: Thanks for the recommendations. Yes I have to use jdk 6 and its a academic project. I was told not to encrypt and salt the password. I labeled it so everyone knows that its plain text. This will take me a second to digest.