Pre Java 5 collections and the unwillingness to change

Question

We have several areas where code like this can be seen

public Map extractData(ResultSet rs) throws SQLException, DataAccessException {
     Map m = new HashMap();
         while(rs.next()){
                    Jurisdiction j = new Jurisdiction();
                    m.put("code",rs.getLong(1) );
                    m.put("type",rs.getString(2));
                    m.put("id",rs.getLong(3) )
         }
     return m;
        }

which works perfectly fine. The thing is I am hard-pressed to find a reason to re-write this in a "generic" way and if I do what "real" benefit does Map<String,Object> m give me (other than bounding to String keys)?

Answer 1

It gives you improved readability and maintainability. Support and maintenance is traditionally the most expensive aspect of software development. Spend some time up front and hopefully save yourself a few bucks down the road.

Answer 2

If the code works, there probably isn't much to be gained from rewriting it. However, aside from the compile-time type-checking, the other reason you might want to switch to using generics is that it makes the code self-documenting. It makes it explicit about what types it is using.

Answer 3

Type safety, which mostly means error detection during compile time.

This means simply better code and better maintainability, because future changes to the codebase are less likely to break something.

But: Whether you should re-write the code or not is a decision only you can make.

If everything works just fine now and you know your user-base (maybe it is only you ;-) and that they know what they do.....why invest the cost of change? Also think about the possibilty of introducing new bugs and subtle errors when you make the changes (though IDE support like IntelliJ IDEA has [generify] helps).

Answer 4

If there are R readers of the code, and it takes them N milliseconds to realize that m is Map<String,Object>, then using generics would save approx. R x N milliseconds. Over time, this is significant. Also, you are improving the doc to the client (with something real, not just Javadoc).

Answer 5

You may be happy with that code and understand it - but what about someone else who comes along in a couple of years and wants to maintain it?

Any extra help re: types is useful in a statically typed language. Your argument about <String, Object> is valid so why not create a value object that is strongly typed and add that to the map with a unique key?

Answer 6

The benefit is that any callers can assume, not because they're familiar with the code or because they're pretty sure that they know how the map is constructed, but because the language enforces it, that the keys are strings. The heterogeneous nature of the values is also explicitly specified.

It may seem silly, as you know that the code already works, but I've been in situations where you have to integrate legacy code or libs into a generic app, and the boundary between the known types and the unknown maps is painfully obvious. To the consumer of the map, you either have to

• assume the key is a string (bad practice)
• dig through the source (if you can) to convince yourself that the keys are always strings (avoiding this is reason enough to use generics)
• hack some kind of cast to make sure you get a string at the end, because you don't trust the code (e.g. String key=keyObject+"");

You'd probably also waste time trying to decide if the values were typed as well.

If you really, really want to avoid the generics I'd at the very least put a comment on the function that describes the return types.

Answer 7

Best practice is to use List<Data>. Have a Data class with code, type and id properties and the appropriate public getters/setters which is just a javabean class which represents a single row of the table. Oh, you may change the classname "Data" to whatever the actual data represents.

public class Data {
    private Long code;
    private String type;
    private Long id;
    // Add (or generate) getters/setters here.
}

And use it as follows:

private static List<Data> listData(ResultSet resultSet) throws SQLException {
    List<Data> list = new ArrayList<Data>();
    while (resultSet.next()){
        Data data = new Data();
        data.setId(resultSet.getLong("id"));
        data.setCode(resultSet.getLong("code"));
        data.setType(resultSet.getString("type"));
        list.add(data);
    }
    return list;
}

Alternatively you can also use Map<Long, Data> where the Long key represents the "primary key" of the Data object.

private static Map<Long, Data> mapData(ResultSet resultSet) throws SQLException {
    Map<Long, Data> map = new HashMap<Long, Data>();
    while (resultSet.next()){
        Data data = new Data();
        data.setId(resultSet.getLong("id"));
        data.setCode(resultSet.getLong("code"));
        data.setType(resultSet.getString("type"));
        map.put(data.getId(), data);
    }
    return map;
}

Note that I changed the indexes by column names because using indexes often indicates that you're doing a SELECT * FROM table and relying on the DB-specific ordering of columns. This is a bad practice, you should actually be doing a SELECT id, code, type FROM table. I also changed the modifier to private static because in real you should never be passing the ResultSet outside the DAO class where it is been used. Always acquire and close Connection, Statement and ResultSet in the very same method block.

For more insights about doing the basic DAO the right way you may find this article useful as well.

Answer 8

(This is relevant to your question, give it a sec)

If you look through the Javadocs, you will never (correct me if I'm wrong) find a method outside Util that takes or returns a collection.

At first I couldn't understand this--they must use collections all over.

The thing is, a collection makes a LOUSY api. It's unsafe (how do you guarantee that other threads aren't changing it? How do you ensure that nobody deletes a key element? How do you ensure that it is kept in sync with other collections/data/...?)

Also, note that what you wrote is a utility function, not a method (it references no member variables). It is not OO. That happens a lot when you pass collections and it's generally a good indication of poor OO design (the method is in the wrong class).

If you passed your result set to the constructor of an object that contained your collection, you would have much more control over the situation--you would also find that many other utility functions actually belong in that class.

The thing with generics is that once your collections are contained in a fairly restricted class, they are fun but no where near as important--still useful of course.

If you were going to go so far as to refactor your collection to use Generics, you might consider going all the way and making an object that contains it as well.

The refactor to generics itself then becomes really easy--pretty much free. Without it generics is pretty much just a band-aid that leaves huge gaping holes in your design/safety anyway.

Answer 9

The main benefit is to the caller of the method.

public void processData(ResultSet rs) throws SQLException {
     Map m = extractData(rs);
     setType(m.get( ...)); //now what kind of key did that map take again? Have to go look ...
}

It isn't necessarily worth it (especially in stable code). I am doing it now and I found some interesting type errors in the code by doing it. Sure it happened to not trip up a bug in production, but that was just waiting to happen. By forcing the same parameter types throughout, we were able to unearth such potential bugs.

Answer 10

The thing is I am hard-pressed to find a reason to re-write this in a "generic" way

I don't think there is a compelling reason to do that. If you are not creating a new version, or fixing bugs around the code, I think you should better focus on more relevant sections.

I mean, there is a chance ( very high possibility ) that, the "application/library/framework/component/etc" to which this code belongs to, have a number of defect/issues/enhancements yet to be coded.

I would rather have that new functionality using generics, rather than changing this piece.

and if I do what "real" benefit does Map m give me (other than bounding to String keys)?

As of now, absolutely nothing. Some may claim readability but, how often do you go and look at a piece of code which ( quoting you ) "which works perfectly fine"

Now, if that piece of code is in the middle of some section of code that is currently being changed, or that is causing a number of bugs, you may refactor the problematic component and fix this in the process, what you'll gain will be compile time checking + readability. But if it is not the case, I would leave it just like that.