Generic DAO written in Java

Question

I initially thought that this was just an architecture issue so I placed it on programmers as Thoughts on refactoring a generic DAO. Then I asked on codereview meta here and decided to put up the code here.

• Here's a github link if someone wants to see there.
• There are four DAOs - DAODelete, DAOUpdate, DAORead, DAOInsert but I am just putting DAORead and its implementation here so that it is easier to review.
• I have trimmed the code a lot so if something is amiss please tell me and I'll correct it. Otherwise you can see github link as it has complete code(kinda).

---

My architecture is like this

• Abstraction layer of DAO at top
• DataBase specific implementation of DAO BUT Table-independent
• Table dependency is passed down to a lower utility layer
• A Enum is passed down to the lower utility layer by which it gives table specific results
• Table-specific utility classes for the lower utility layer mentioned in the previous point.
• There is a Student pojo that I am omitting because well it is just a pojo. private variables and all. Just note that enrollmentDate is of type java.sql.Date

My concern

• I am satisfied with the upper layer of DAO but I think that at the the lower level specifically at the OracleSpecific.java the things which should have strong coupling have weak coupling. e.g. there are different methods for getting pojo from resultset or getting primary key. Each of these in turn have switch case for calling the functions in lower utility class.
• Although the schema-specific methods are tied together in different utility classes the method call themselves in OracleSpecifics.java do not have any coupling.
• I am thinking whether I should change the enum TableName to contain a specific state. The state I am thinking for the schema-specific lowest level utility classes.

• These specific states contained in enums can be used to change the state of DAO and the DAO can then call the specific functions based on an interface implemented by all such states. Thus depending on the state the behavior can change automatically.

• Is this design decision correct or would it be meaningless?

• Any other thing that I might have overlooked?
• Any thoughts about the design itself?
• Would I lose type safety introduced by generics due to this change?

---

Enums Used

QueryType.java

package aseemEnums;

public enum QueryType {
    READ, DELETE;
}

Databases.java

package aseemEnums;

public enum Databases {
    Oracle;
}

TableName.java

package aseemEnums;

public enum TableName {
    STUDENT_TABLE("STUDENT_TABLE");

    private final String tableName;

    TableName(String tableName) {
        this.tableName = tableName;
    }

    public String toString() {
        return tableName;
    }
}

---

The Abstraction layer

DAOFactory.java

package aseemDao;

import java.sql.PreparedStatement;
import java.sql.ResultSet;
import java.sql.SQLException;

import javax.naming.NamingException;

import aseemEnums.Databases;

public abstract class DAOFactory {

    // Abstract Instance methods
    public abstract DAOInsert getDAOInsert() throws SQLException;

    public abstract DAORead getDAORead() throws SQLException;

    public abstract DAODelete getDAODelete();

    public abstract DAOUpdate getDAOUpdate();

    // Concrete Class Methods
    public static DAOFactory factoryProducer(Databases db)
            throws NamingException {
        switch (db) {
        case Oracle:
            return new OracleFactory();
        default:
            return null;
        }
    }

    static void closeAll(PreparedStatement ps, ResultSet rs) {
        try {
            rs.close();
        } catch (Exception e) {
        }
        try {
            ps.close();
        } catch (Exception e) {
        }
    }
}

DAORead.java

package aseemDao;

import java.sql.Connection;
import java.sql.SQLException;
import java.util.List;

import aseemEnums.TableName;

public interface DAORead {

    public abstract <T> List<T> getAll(Connection con, TableName tableName)
            throws SQLException;

    public abstract <T> List<T> getAllForInput(Connection con,
            TableName tableName, String columnName, String searchValue)
            throws SQLException;

    public abstract <T> T getPojoForPrimarKey(Connection con,
            TableName tableName, String primaryKey) throws SQLException;

    public abstract <T> boolean alreadyExisting(Connection con,
            TableName tableName, String primaryKey) throws SQLException;

    public abstract <T> boolean alreadyExisting(Connection con,
            TableName tableName, T currentPojo) throws SQLException;
}

---

Concrete Implementation of DAO's abstraction

OracleFactory.java

package aseemDao;

import java.sql.SQLException;

public class OracleFactory extends DAOFactory {

    @Override
    public DAOInsert getDAOInsert() throws SQLException {
        return new OracleInsert(this);
    }

    @Override
    public DAORead getDAORead() throws SQLException {
        return new OracleRead(this);
    }

    @Override
    public DAODelete getDAODelete() {
        return new OracleDelete(this);
    }

    @Override
    public DAOUpdate getDAOUpdate() {
        return new OracleUpdate(this);
    }
}

OracleRead.java

package aseemDao;

import java.sql.Connection;
import java.sql.PreparedStatement;
import java.sql.ResultSet;
import java.sql.SQLException;
import java.util.ArrayList;
import java.util.List;

import aseemEnums.QueryType;
import aseemEnums.TableName;

public class OracleRead implements DAORead {

    DAOFactory fac = null;

    OracleRead(DAOFactory fac) throws SQLException {
        this.fac = fac;
    }

    @SuppressWarnings("unchecked")
    public <T> List<T> getAll(Connection con, TableName tableName)
            throws SQLException {
        List<T> list = new ArrayList<T>();
        PreparedStatement ps = null;
        ResultSet rs = null;
        try {
            ps = con.prepareStatement("select * from " + tableName);
            rs = ps.executeQuery();
            while (rs.next()) {
                list.add((T) OracleSpecifics
                        .getPojoFromResultSet(tableName, rs));
            }
        } finally {
            DAOFactory.closeAll(ps, rs);
        }
        return list;
    }

    @SuppressWarnings("unchecked")
    public <T> List<T> getAllForInput(Connection con, TableName tableName,
            String columnName, String searchValue) throws SQLException {
        List<T> list = new ArrayList<T>();
        PreparedStatement ps = null;
        ResultSet rs = null;
        try {
            ps = con.prepareStatement("SELECT * FROM " + tableName + " WHERE "
                    + columnName + " LIKE '%" + searchValue + "%'");
            rs = ps.executeQuery();
            while (rs.next()) {
                list.add((T) OracleSpecifics
                        .getPojoFromResultSet(tableName, rs));
            }
        } finally {
            DAOFactory.closeAll(ps, rs);
        }
        return list;
    }

    @Override
    public <T> T getPojoForPrimarKey(Connection con, TableName tableName,
            String primaryKey) throws SQLException {
        T currentPojo = null;
        PreparedStatement ps = null;
        ResultSet rs = null;
        try {
            String queryString = OracleSpecifics.queryString(tableName,
                    primaryKey, QueryType.READ);
            ps = con.prepareStatement(queryString);
            rs = ps.executeQuery();
            if (rs.next()) {
                currentPojo = OracleSpecifics.getPojoFromResultSet(tableName,
                        rs);
            }
        } finally {
            DAOFactory.closeAll(ps, rs);
        }
        return currentPojo;
    }

    @Override
    public <T> boolean alreadyExisting(Connection con, TableName tableName,
            String primaryKey) throws SQLException {
        if (getPojoForPrimarKey(con, tableName, primaryKey) != null) {
            return true;
        } else {
            return false;
        }
    }

    @Override
    public <T> boolean alreadyExisting(Connection con, TableName tableName,
            T currentPojo) throws SQLException {
        String primaryKey = OracleSpecifics.<T> getPrimaryKey(tableName,
                currentPojo);
        if (alreadyExisting(con, tableName, primaryKey) == false) {
            return false;
        } else {
            return true;
        }
    }
}

---

The lower utility layer in DAO

OracleSpecifics.java

package aseemDao;

import java.sql.Connection;
import java.sql.PreparedStatement;
import java.sql.ResultSet;
import java.sql.SQLException;

import aseemEnums.QueryType;
import aseemEnums.TableName;

import aseemPojo.Student;

public class OracleSpecifics {

    // These functions don't call any table-specific functions
    static String queryString(TableName tableName, String keyValue,
            QueryType type) {
        String firstHalf = null;
        switch (type) {
        case READ:
            firstHalf = "select * from " + tableName + " where ";
            break;
        case DELETE:
            firstHalf = "DELETE from " + tableName + " where ";
            break;
        default:
        }

        switch (tableName) {
        case STUDENT_TABLE:
            return firstHalf + "STUDENT_ID='" + keyValue + "'";
        default:
            return null;
        }
    }

    static <T> String getPrimaryKey(TableName tableName, T currentPojo) {

        switch (tableName) {
        case STUDENT_TABLE:
            return ((Student) currentPojo).getStudentId();
        default:
            return null;
        }
    }

    // These functions call table-specific functions
    @SuppressWarnings("unchecked")
    static <T> T getPojoFromResultSet(TableName tableName, ResultSet rs)
            throws SQLException {

        switch (tableName) {
        case STUDENT_TABLE:
            return (T) SpecStudent.getPojo(rs);
        default:
            return null;
        }
    }
}

One of the Table-Specifics utility Classes

SpecStudent.java

package aseemDao;

import java.sql.Connection;
import java.sql.PreparedStatement;
import java.sql.ResultSet;
import java.sql.SQLException;

import aseemEnums.TableName;
import aseemPojo.Student;

public class SpecStudent {
    public static Student getPojo(ResultSet rs) throws SQLException {
        Student currentStudent = new Student();
        currentStudent.setStudentId(rs.getString("Student_Id"));
        currentStudent.setRollNo(rs.getString("Roll_No"));
        currentStudent.setStudentName(rs.getString("Student_Name"));
        currentStudent.setAddress(rs.getString("Address"));
        currentStudent.setEmail(rs.getString("Email"));
        currentStudent.setContactNumber(rs.getString("Contact_Number"));
        currentStudent.setGuardianName(rs.getString("Guardian_Name"));
        currentStudent.setEnrollmentDate(rs.getDate("Enrollment_Date"));
        return currentStudent;
    }
}

Answer 1

General

• Replace if (<expr>) return true; else return false; with return <expr>;.

• Change all but the first letter of each acronym into lowercase: DaoFactory. While it looks strange at first, they're easier to type, you'll quickly get used to it, and you won't get confused when they're combined as in MySQLJDBCDAO. This simple, consistent rule eliminates a lot of guesswork.

• Be consistent with your plural vs. singular naming. QueryType, TableName and . . . Databases? Code completion will obviously keep you from getting it wrong, but that will slow you down after a while.

• Spell check your code. Yes, you have to do it manually, but there's no reason to let getPojoForPrimarKey make it into a public interface. And h.j.k. is right here: switch "pojo" out for "entity" or something more explanatory.

• Don't leak exceptions from the specific implementations such as SQLException because you're limiting yourself to SQL databases using checked exceptions. Create DaoException and map them with a utility class a la Hibernate and Spring.

Java

• Java package names are typically all lowercase and form a tree structure: aseem.enums, aseem.dao, and aseem.pojo. They also tend to avoid plurals, so aseem.enum instead of aseem.enums.

• Use StringBuilder instead of concatenating strings in many steps. Every x + y + ... + z creates a new StringBuilder, appends the strings, and packages the result under the hood. Splitting this across statements results in extra temporary builders and immutable strings which causes churn in the garbage collector.

• Every method in an interface is by definition public and abstract, so you may remove those modifiers. This is more a personal preference, and in those cases I tend to lean toward laziness wherever practical. Any superfluous information that applies to everything is noise.

• Don't catch Exception except at very high-levels such as an event loop or thread's run method. DAOFactory.closeAll can catch SQLException to let unrelated problems bubble up. The reason is that you may inadvertently catch an exception (e.g. InvalidStateException) that you aren't prepared to handle. It's safe to ignore a SQLException in closeAll, but not other types of exceptions that may signal real problems. Thus, change both catch clauses to SQLException.

DAO Design

• Does the set of table names really deserve to be decided at compile time? Why not column names, too? I didn't see any place where you're switching on the table [oh, see next item], and if you are that should be moved into a proper class with polymorphic behavior instead. An enumeration should model a truly fixed set of items. QueryType is a great example.

• OracleSpecifics seems like it will become a monster as it grows to deal with a) Oracle and b) every table. Create a proper Table class with either a subclass for each table or read the configuration from a file so that OracleSpecifics can be 100% Oracle-specific and you don't have to repeat it in MySqlSpecifics and SqlServerSpecifics.

• What does alreadyExisting mean? Does this query for the database to see if the primary key is already there? How about exists? Similarly with getAllForInput; make the name more explicit such as getForColumn to distinguish it from getForColumns that takes a column/value map. I've always been fond of findBy... for query methods that may return no results and getBy... for methods that return a single result or throw an exception.

• Why does the OracleRead constructor declare that it throws SQLException when it cannot?

Answer 2

I'll leave the actual generic DAO discussions out of this answer for now, and just offer my two cents on coding styles...

1. You can use better method names instead of getPojo... Pojo is well-understood but still quite an informal term, and you'll probably attract equal comments whether to keep it all caps or not - it's an acronym afterall. The Spring framework calls their helper classes RowMappers with a mapRow method, so maybe you want to follow something like that. http://docs.spring.io/spring/docs/3.0.x/api/org/springframework/jdbc/core/RowMapper.html

2. You can inline your conditional checks in the return statements as such:

   @Override
   public <T> boolean alreadyExisting(Connection con, TableName tableName,
           String primaryKey) throws SQLException {
       return getPojoForPrimarKey(con, tableName, primaryKey) != null;
   }
   
   @Override
   public <T> boolean alreadyExisting(Connection con, TableName tableName,
           T currentPojo) throws SQLException {
       return alreadyExisting(con, tableName, OracleSpecifics.<T> getPrimaryKey(tableName, currentPojo));
   }
   

3. Actually, I'm not that sure whether you really need to map the Pojo in order to determine if a row with the primary key exists or not... Perhaps there's a simpler way?

edit:

• Your Oracle* classes don't really have anything Oracle-specific now, I'm not sure if that is intended because things are still work-in-progress. Or do you mean you will be creating new classes for other RDBMS software like MySQL, MariaDB etc. with their own dialect?
• DAOs should not need to keep 'states' beyond the current ResultSet object that it is doing the mapping for. You may need to re-consider your implementation of the TableName enum to contain a specific state. More clarification is needed here.
• After another quick reading, you may want to consider the 'generic-ness' of your approach, because it appears that adding new classes (aka entities) to pass to your DAO framework requires a Spec<Table> class (or updating your existing SpecStudent) for the actual implemention (e.g. calling all the getter methods on your Student class), and updating your TableName enum as well. Maybe you can also consider using reflection for the implementation, so that there is less code to update in the long run?