DAOs for Person, Employee, Staff, etc

Question

I have an abstract PersonDao class, extended by several other DAO classes, for example EmployeeDao, StaffDao.

The non-abstract DAOs correspond to different tables. The tables have some common fields, and also some unique fields. Operations on the common fields are implemented in the abstract parent PersonDao class, operations on the unique fields are implemented in the specialized DAOs.

The parent DAO looks like this:

public abstract class PersonDao {

    private SimpleJdbcTemplate simpleJdbcTemplate;

    public PersonDao() {
        final String tableName = getTableName();
        fieldNamesCSV = getFieldNames();

        String select = String.format("SELECT %s FROM %s ", fieldNamesCSV, getTableName());
    }

    abstract String getTableName();

    abstract String getHistoricalTableName();

    public void setDataSource(DataSource dataSource) {
        this.simpleJdbcTemplate = new SimpleJdbcTemplate(dataSource);
    }

    // more methods, common to all persons
}

An example specialized DAO looks like this:

public class EmployeeDao extends PersonDao {

    private static final String TABLE_NAME = "EMPLOYEE";
    private static final String HISTORICAL_TABLE_NAME = "EMPLOYEE_HIST";

    @Qualifier("employeeDS")
    @Override
    public void setDataSource(DataSource dataSource) {
        super.setDataSource(dataSource);
    }

    @Override
    String getTableName() {
        return TABLE_NAME;
    }

    @Override
    String getHistoricalTableName() {
        return HISTO_TABLE_NAME;
    }

    // more methods, unique to employees
}

My question is about the code duplication in the specialized DAOs. They all have the boilerplate code of setting the data source (configured by Spring), and the overridden methods for the table names, so that the common methods in the parent class work with any table. Code quality analysis tools like Sonar detect and flag an error for these blocks.

• Do you suspect something wrong with my design that necessitates the boilerplate code? If so, please give some tips to improve it.
• If the design looks legit, do you see a way to reduce the duplication? Moving the table name constants to the methods as hard-coded strings would fool the code analysis tool, but that's hardly the solution.
• Any other tips come to mind?

UPDATE

As @pinoniq pointed out, the setDataSource was smelly. I vaguely remembered that Spring forced me to do it that way, and differentiate the specialized classes with the @Qualifier annotation. It seems that's not the case, I could move data source to the constructor of the specialized classes, and everything seems to work well.

Answer 1

Smelly code

The parent method smells:

public void setDataSource(DataSource dataSource) {
    this.simpleJdbcTemplate = new SimpleJdbcTemplate(dataSource);
}

You ask a DataSource, but in fact you need a SimpleJdbcTemplate. this smells... Let's assume I need to test your code. I run my unit tests and pass in a DataSource, It somehow throws this really weird exception class SimpleJdbcTemplate not found.

This smells because now, your DAO classes are tightly coupled with your SimpleJdbcTemplate class. What if the constructor changes? what if you whant to use something different then SimpleJdbc? ... If you need a class, ask for it.

the fix

public void setSimpleJdbcTemplate(SimpleJdbcTemplate template) {
    this.simpleJdbcTemplate = template;
}

I don't trust my parent

The override in the child classes smell even harder. Your setDataSource method does exactly the same, but yet you @override ? But the only thing you do is delegate everything to the parent. why? what is the logic here?

public, static, private, mess

Some methods are declared public, some are not. And whats it with those private statics? what is the point of those?

Wouldn't it be better to have just one private variable called table that holds the table where you DAO class is looking for persons? And then simply create 2 Objects, one for your new table and one for you historical table?

What happened to this?

Sometimes you use this. in your code, and sometimes you don't? smelly

public PersonDao() {
    final String tableName = getTableName(); // what happened to this?
    fieldNamesCSV = getFieldNames(); // what happened to this?
    String select = String.format("SELECT %s FROM %s ", fieldNamesCSV, getTableName()); //again, where is this?
}

What does your class need

Looking at your constructor, Your class needs nothing. Not even a datasource, or table name or ...

Again, if you need it, ask for it.

The best code quality analysis tool is 'grandparents'

Code should read like poetry. You know what is going on, but it doesn't feel like perfectly written English. If your code is readable, it's good. If it reads a little bit weird, it smells...

for instance your setDataSource method:

The public returns void method setDataSource needs a DataSource object. this simepleJdbcTemplate is now a new SimpleJdbcTemplate that needs a DataSource object.

smells right?

Or your @Override method:

I override the parrent method setDataSource. The new method needs a DataSource and passes it to the parent.

hmmm...

Answer 2

Minor Points:

• Your child classes don't need to override the setDataSource() method because this method is already implemented on the abstract PersonDao class.

• What use is a data access object without a data source? You probably want to pass your dataSource object into the PersonDao class as a constructor argument.

Larger Refactorization Idea:

Given the small amount of code in your question, it seems like this is a good example of when to prefer composition over inheritance. http://en.wikipedia.org/wiki/Composition_over_inheritance. All the abstract methods on your PersonDao classes can be wrapped up in other objects to be injected into a particular DataAccessObject as a dependency.

Instead of inheriting from PersonDao, EmployeeDao should take all the interface/components in needs for data-access as constructor arguments. Specifically, the EmployeeDao constructor should probably take components like an IDataSource interface and a TableFieldInfo class. These dependencies can be injected at compile time in your Spring config file allowing you to make many data-access related changes without recompiling. Then you will create some public static helper methods that will construct the specific data access objects that you need with the right configuration parameters injected eg:

PersonDao person = DataAccess.Person(); 

This will eliminate the code duplication you asked about and make your data access objects a lot more configurable and extensible down the road.