1
\$\begingroup\$

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;
        }
    }
}
\$\endgroup\$

2 Answers 2

2
\$\begingroup\$

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>();       
    }
}
\$\endgroup\$
3
  • \$\begingroup\$ You can comment now =D \$\endgroup\$
    – Pimgd
    Oct 30, 2014 at 9:50
  • \$\begingroup\$ hehe yeah true :) thanks for the edits, i'm still learning the proper syntax. the @<username> notation is a good one. I promise to improve my writing style \$\endgroup\$
    – Joeblade
    Oct 31, 2014 at 9:02
  • \$\begingroup\$ @Username doesn't actually work in answers, but since it does work that way in chat and in comments, we tend to just use it like that in answers too. It shows that you're talking about someone else. \$\endgroup\$
    – Pimgd
    Oct 31, 2014 at 9:06
1
\$\begingroup\$

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.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.