DAOs for Person, Employee, Staff, etc

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 n

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...

StackExchange Code Review Q#57172, answer score: 6