Spring Boot DAO

Question

I am using spring boot with NamedParameter JDBC. Please suggest any improvements to my code.

This DAO first determines which datasource to connect to which is based off the condition of the request data. If the request date is over 30 days it will connect to the archival database. Then it determines the sql to be used by an UNION operator from three different tables.

---

@Repository
public class SmsSearchDAOImpl implements SmsSearchDAO {
    //~ Instance fields ----------------------------------------------------------------------------

    @Autowired
    @Qualifier("gudTemplate")
    private NamedParameterJdbcTemplate jdbcTemplate;

    @Autowired
    @Qualifier("archivalTemplate")
    private NamedParameterJdbcTemplate archivalJdbcTemplate;

    @Autowired
    private MessageProperties prop;

    //~ Methods ------------------------------------------------------------------------------------

    /**
     * If the request coming in is older than 30 days then establish a connection to the the archival database only. 
     */
    private List<MessagesDTO> determineDataSource(String sql, Map<String, Object> params, MessagesRowMapper rowMapper, boolean archivalFlag) {      
        if(archivalFlag) {
            return archivalJdbcTemplate.query(sql, params, new MessagesRowMapper());
        } else {
            return jdbcTemplate.query(sql, params, new MessagesRowMapper());
        }
    }

    /**
     * Include JavaDocs please!
     *
     * @param bean DOCUMENT ME!
     *
     * @return DOCUMENT ME!
     */
    @Override
    public List<MessagesDTO> findByCustId(MessagesDTO rtDto, String startDate, String endDate, boolean archivalFlag) {
        String requestType = rtDto.getRequestType();
        String appId = rtDto.getAppId();
        Map<String, Object> params = determineParams(rtDto, startDate, endDate);
        String formattedSQL = MessageFormat.format(BATCH_SMS_LITE_SQL_CUSTID, new Object[] {prop.getProperty("batchmessage.schema")});
        String SQL = determineSQL(CCC_MESSAGES_SQL_CUSTID, BATCH_MESSAGES_SQL_CUSTID, formattedSQL, requestType, appId);
        return determineDataSource(SQL, params, new MessagesRowMapper(), archivalFlag);
    }

    /**
     * Include JavaDocs please!
     *
     * @param bean DOCUMENT ME!
     *
     * @return DOCUMENT ME!
     */
    @Override
    public List<MessagesDTO> findByMtn(MessagesDTO rtDto, String startDate, String endDate, boolean archivalFlag) {     
        String requestType = rtDto.getRequestType();
        String appId = rtDto.getAppId();
        Map<String, Object> params = determineParams(rtDto, startDate, endDate);
        String formattedSQL = MessageFormat.format(BATCH_SMS_LITE_SQL_MDN, new Object[] {prop.getProperty("batchmessage.schema")});
        String SQL = determineSQL(CCC_MESSAGES_SQL_MDN, BATCH_MESSAGES_SQL_MDN, formattedSQL, requestType, appId);
        return determineDataSource(SQL, params, new MessagesRowMapper(), archivalFlag);
    }

    /**
     * Include JavaDocs please!
     *
     * @param bean DOCUMENT ME!
     *
     * @return DOCUMENT ME!
     */
    @Override
    public List<MessagesDTO> findByCustIdAndMtn(MessagesDTO rtDto, String startDate, String endDate, boolean archivalFlag) {        
        String requestType = rtDto.getRequestType();
        String appId = rtDto.getAppId();
        Map<String, Object> params = determineParams(rtDto, startDate, endDate);
        String formattedSQL = MessageFormat.format(BATCH_SMS_LITE_SQL, new Object[] {prop.getProperty("batchmessage.schema")});
        String SQL = determineSQL(CCC_MESSAGES_SQL_MDN_CUSTID, BATCH_MESSAGES_SQL_MDN_CUSTID, formattedSQL + BATCH_SMS_LITE_SQL_MDN_CUSTID, requestType, appId);
        return determineDataSource(SQL, params, new MessagesRowMapper(), archivalFlag);
    }

    //~ Inner Classes ------------------------------------------------------------------------------


    private static class MessagesRowMapper implements RowMapper<MessagesDTO> {
        //~ Methods --------------------------------------------------------------------------------

        @Override
        public MessagesDTO mapRow(ResultSet rs, int rowNum) throws SQLException {
            MessagesDTO cccMsg = new MessagesDTO();
            cccMsg.setAppId(rs.getString("APPLICATION_ID"));
            cccMsg.setMdn(rs.getString("NOTIFICATION_ADDRESS"));
            cccMsg.setDeliveryDate(rs.getString("NOTIFICATION_DELIVERY_DATE"));
            cccMsg.setRequestType(rs.getString("NOTIFICATION_TYPE"));
            return cccMsg;
        }
    }

    private String determineSQL(String ccesSql, String batchSql, String smsLiteSql, String requestType, String appId) {
        final String AND = " AND ";
        StringJoiner sqlCCCMsgs = new StringJoiner(AND, ccesSql, "");
        StringJoiner sqlBatchMsgs = new StringJoiner(AND, batchSql, "");
        StringJoiner sqlBatchSmsLiteMsgs = new StringJoiner(AND, smsLiteSql, "");
        if((requestType != null) && !requestType.isEmpty()) {
            sqlCCCMsgs.add(REQUEST_TYPE);
            sqlBatchMsgs.add(REQUEST_TYPE);
            sqlBatchSmsLiteMsgs.add(REQUEST_TYPE);
        }
        if((appId != null) && !appId.isEmpty()) {
            String[] appIdSplit = appId.split(",");
            if(appIdSplit.length > 1) {
                sqlCCCMsgs.add(APP_ID_IN);
                sqlBatchMsgs.add(APP_ID_IN);
                sqlBatchSmsLiteMsgs.add(APP_ID_IN);
            } else {
                sqlCCCMsgs.add(APP_ID);
                sqlBatchMsgs.add(APP_ID);
                sqlBatchSmsLiteMsgs.add(APP_ID);
            }
        }
        return sqlCCCMsgs.toString() + UNION + sqlBatchMsgs.toString() + UNION + sqlBatchSmsLiteMsgs;
    }

    private Map<String, Object> determineParams(MessagesDTO rtDto, String startDate, String endDate) {
        Map<String, Object> params = new HashMap<>();
        String mdn = rtDto.getMdn();
        String custId = rtDto.getCustId();
        String requestType = rtDto.getRequestType();
        String appId = rtDto.getAppId();
        params.put("mdn", mdn);
        params.put("custId", custId);
        params.put("startDate", startDate);
        params.put("endDate", endDate);
        if((requestType != null) && !requestType.isEmpty()) {
            params.put("request", requestType);
        }
        if((appId != null) && !appId.isEmpty()) {
            params.put("appId", appId);
        }
        return params;
    }   
}

Answer 1

It doesn't look too bad. To add to the other answers:

• Please put a space after the if keyword.
• If you are not going to use javadoc then remove the comments.
• Joining SQL strings is usually a very bad practice because it can make you vulnerable to SQL injection. Now if this really is an issue is impossible to tell, because you didn't include the actual queries.

Answer 2

In order of appearance:

1. Despite of what your given tool says, don't use its naming convention if you can avoid it. What are jdbcTemplate and archivalJdbcTemplate actually? You said they were datasources (and they are kind of that, just an access wrapper of it), why don't you call it like that (smsDatasource and archiveSmsDatasource)?
2. In the method of determineDataSource, you are not returning your datasource, you are returning a list of items as the result of a query you have run on that datasource. You need to change the method name or the logic within the method. I'd just create a method with name getDataSource() to choose the right NamedParameterJdbcTemplate to be used. Then, what you have left in your old method has almost the same functionality than NamedParameterJdbcTemplate.query().
3. Try to avoid using acronyms or abbreviations. A few examples of this would be: what is custId? what is rtDto? what is mtn?... In the old days, fellow programmers didn't have IDE's that autocomplete the stuff you half-typed, neither they had wide screens... They have real limitations that we no longer have and we can now write descriptive yet concise stories in our sourcecode.
4. It looks like you have some written queries as instance fields, but the names are not descriptive enough, so I cannot know what those queries are doing, and that is deceptive and dangerous.

Answer 3

There is no real possibility to improve this code as it mostly consists of sequences of statements. You have one method with a condition depth of 2 and two with a condition depth of one. You do not even have a loop.

Maybe someone will complain about names and abbreviations.

The code you provide is code embedded in a framework. So the most work is done by the framework and the code is simple as it reflects a simple usecase.

And is it good code? If you asked me: This is too less and simple code to make such a judgement. We cannot know as this depends on the context the provided code is used.