Intended to return account address using account ID, which uses Spring Stored Procedure class

Question

I have not used Springs stored procedure class before to return data and I was hoping someone would be able to check this for me..

If it will A. Return data, B. Can be improved. C. If I wanted to return multiple rows how would this need to be changed?

public class AccountAddress extends StoredProcedure implements DAOConstants{

    private static final String STOREDPROC = "storedProc";
    private static final String CURSOR = "cursor";

    public AccountAddress() {
        super(Config.getJdbcTemplate, STOREDPROC);
        declareParameter(new SqlParameter("accountID", OracleTypes.VARCHAR);
        declareParameter(new SqlOutParameter(CURSOR,OracleTypes.CURSOR, new AccountAddressMapper());
        setFunction(false);
        compile();
    }

    public List<String> getAccountAddress(String accountId) {
        Map<String,Object> params = new HashMap<~>();
        Map<String,Object> results = new HashMap<~>();
        List<String> data = new LinkedList<~>();
        try{
            params.put("accountId",accountId);
            results = execute(params);
            data = (List<String>) results.get(0);
        } catch (DataAccessException e) {
            // log errors  
        }
        return data;
    }

    public class AccountAddressMapper implements RowMapper<List<String>> {
        @Override
        public List<String> mapRow(Resultset rs, int i) throws SQLException{
            List<String> data = new ArrayList<~>();
            data.add(rs.getString(4));
            data.add(rs.getString(5));
            data.add(rs.getString(6));
            data.add(rs.getString(7));
            data.add(rs.getString(8));
            return data;
        }
    }
}

Answer 1

As @Pimgd mentioned in his answer: You are setting defaults just to overwrite them. this sounds like you are not certain what is going to happen in your code and you want to 'make sure'. It's better to read your code over a few times and make sure you know what will happen, rather than set values you won't need. To another developer it will sound like there is a situation where you will need the results map you assigned.

1. Don't declare parameters (with defaults) if you don't use them outside of the scope you need them in. when a method grows, the distance between the parameter (at top of try) and the place it gets used grows, making it more likely that it will get reused (bad) or that it gets left behind because developers don't notice it isn't used anymore.

2. Use the exception, not just to report an error. either return null or return an empty List, whichever you want. (or if you dislike return statements in a catch, set the data field to null/empty list)

3. If you are going to return the dataset without mutation, why not return it straight away?

I would suggest:

public List<String> getAccountAddress(String accountId) {
    try{
        Map<String,Object> params.put("accountId",accountId);
        Map<String,Object> results = execute(params);
        return (List<String>) results.get(0);
    } catch (DataAccessException e) {
        // log errors 
        return new LinkedList<String>();       
    }
}

Answer 2

Something I noticed:

    Map<String,Object> params = new HashMap<~>();
    Map<String,Object> results = new HashMap<~>();
    List<String> data = new LinkedList<~>();
    try{
        params.put("accountId",accountId);
        results = execute(params);
        data = (List<String>) results.get(0);
    } catch (DataAccessException e) {
        // log errors  
    }

You declare results here to be a new HashMap, but then you overwrite the value without using it. Seems unnecessary to me. I'd set results to null.

As for the data variable in this snippet, why do you instance a new LinkedList when the return type of the row mapper is an ArrayList instead? I'd keep the types used uniform.