Java and SQL : return null or throw exception?

Answer 1

I'd avoid the following

   sql.append("SELECT * FROM ").append("dogs_table");
   sql.append(" WHERE ").append(colName).append("='");
                        sql.append(colValue).append("'");

and instead use a PreparedStatement with its associated parameter setter methods (setString()) etc. This will prevent problems with values for colValue having quotes, and SQL injection attacks (or more generally, colValue forming some SQL syntax).

I would never return a null if the collection was merely empty. That seems very counter-intuitive, and completely unexpected from the client's point of view.

I wouldn't recommend returning a null in error conditions, since your client has to explicitly check for this (and will probably forget). I would return an empty collection if need be (this may be analogous to your comment re. a null object), or more likely throw an exception (depending on the circumstances and the severity). The exception is useful in that it will carry some information relating to the error encountered. Null tells you nothing.

What should you do if encountering a problem whilst building a Dog object ? I think that depends on how robust and resilient you want your application to be. Is it a problem to return a subset of Dogs, or would it be completely catastrophic and you need to report this ? That's an application requirement (I've had to cater for either scenario in the past - best-effort or all-or-nothing).

A couple of observations. I'd use HashMap rather than the old Hashtable (synchronised for all access and , more importantly, not a proper Collection - if you have a Collection you can pass it to any other method expecting any Collection), and StringBuilder over StringBuffer for similar reasons. Not a major problem, but worth knowing.

Answer 2

Null Object Pattern is a design pattern where you always return an object to avoid NPE:s and any null checks in your code. In your case this means that instead of returning null, return an empty Hashtable<Long, Dogs> instead.

The reasoning is that since it's a collection and your other code will access it as such, it won't break down if you return an empty collection; it won't be iterated through, it won't contain anything surprising, it wont cause NPE:s to be thrown and whatnot.

To be precise, a Null Object is a special implementation of a class/interface which does absolutely nothing and thus has no side effects of any kind. Because of its nature of not being null using it will make your code cleaner since when you know that you'll always get an object from your method calls no matter what happens inside the method you don't even have to check for nulls nor make code reacting to them! Because Null Object doesn't do anything, you can even have them as singletons just lying around and thus save memory by doing that.

Answer 3

If an error occurs, thow an exception. If there's no data, return an empty collection, not a null. (Also, generally you should return the more generic 'Map', not the specific implementation),

Answer 4

You can significantly reduce the amount of boilerplate JDBC code by using Spring-JDBC instead of plain-old JDBC. Here's the same method rewritten using Spring-JDBC

public static Hashtable<Long, Dogs> getSomeDogs(String colName, String colValue) {

    StringBuffer sql = new StringBuffer();
    sql.append("SELECT * FROM ").append("dogs_table");
    sql.append(" WHERE ").append(colName).append("='");
    sql.append(colValue).append("'");

    Hashtable<Long, Dogs> result = new Hashtable<Long, Dogs>();

    RowMapper mapper = new RowMapper() {

        public Object mapRow(ResultSet rs, int rowNum) throws SQLException {
            Dogs dog = new Dogs();
            //...initialize the dog from the current resultSet row
            result.put(new Long(dog.getId()), dog);
        }
    };
    (Hashtable<Long, Dogs>) jdbcTemplate.queryForObject(sql, mapper);
}

Spring takes care of:

1. Iterating over the ResultSet
2. Closing the ResultSet
3. Handling exceptions consistently

As others have mentioned you really should use a PreparedStatement to construct the SQL instead of a String (or StringBuffer). If for some reason you can't do this, you could improve the readability of the query by constructing the SQL like this instead:

    String sql = 
        "SELECT * FROM dogs_table " +
        "WHERE " + "colName" + " = '" + colValue + "'";

Answer 5

You ask five questions

1. What ways do u suggest on improving this technique of returning some dogs, with some attribute?

Several, actually.

• Your method is static - this isn't terrible, but leads to you using another static "executeQuery", which smells of Singleton to me...
• The class "Dogs" violates an OO naming practice - plural nouns don't make good class names unless one instance of the class holds a collection of things - and it appears that Dogs is actually "Dog".
• HashTable is all but deprecated. HashMap or ConcurrentHashMap give better performance.
• I can't see a reason to create the first part of your query with multiple appends - it's not bad, but it's less readable than it might be, so sql.append ("SELECT * FROM dogs_table WHERE "); makes a more sensible beginning if you're just going to hardcode the selected columns (*) and table name (dogs_table) anyway.

2. rs.next() will return false for a null ResultSet, or will generate an exception

This doesn't seem to be a question, but yes, rs.next() returns false as soon as there's no longer any rows to process.

3. What if, while initializing a dog object from the current row of the ResultSet, something bad happens

If "something bad happens", what you do next is up to you and your design. There's the forgiving approach (return all the rows you can), and the unforgiving (throw an exception). I tend to lean towards the "unforgiving" approach, since with the "forgiving" approach, users won't know that you haven't returned all the rows that exist - just all the ones you'd gotten before the error. But there might be cases for the forgiving approach.

4. I think u will all agree on this one : nothing bad happens, there are no rows on the interrogation, a null value will be return.

This isn't something on which there's an obvious right answer. First, it's not what's happening in the method as written. It's going to return an empty HashTable (this is what was meant by a "null object"). Second, null isn't always the answer in the "no results found" case.

I've seen null, but I've also seen an empty result variable instead. I claim they're both correct approaches, but I prefer the empty result variable. However, it's always best to be consistent, so pick a method of returning "no results" and stick with it.

5. I've noticed people and groups of people saying that one should split the application into layers, or levels of some kind.

This is harder to answer than the others, without seeing the rest of your application.

Answer 6

Do not build SQL queries by concatenating strings, like you're doing:

sql = new StringBuffer();
sql.append("SELECT * FROM ").append("dogs_table");
sql.append(" WHERE ").append(colName).append("='");
sql.append(colValue).append("'");

This makes your code vulnerable to a well-known security attack, SQL injection. Instead of doing this, use a PreparedStatement and set the parameters by calling the appropriate set...() methods on it. Note, you can only use this to set column values, you can't use this to dynamically construct a column name, as you're doing. Example:

PreparedStatement ps = connection.prepareStatement("SELECT * FROM dogs_table WHERE MYCOL=?");
ps.setString(1, colValue);

rs = ps.executeQuery();

If you use a PreparedStatement, the JDBC driver will automatically take care of escaping certain characters that might be present in colValue, so that SQL injection attacks don't work anymore.