3
\$\begingroup\$

This class seems to be able to represent the data stored in any possible database table:

public class DbTable {
    private Set<Map<String, Object>> tblData = new HashSet<Map<String, Object>>();

    public DbTable(Connection dbConn, String tblName) throws Exception {
        PreparedStatement prpStmt = dbConn.prepareStatement("SELECT * FROM " + tblName);
        ResultSet queryResults = prpStmt.executeQuery();
        ResultSetMetaData metaData = queryResults.getMetaData();

        while (queryResults.next()) {
            Map<String, Object> row = new HashMap<String, Object>();

            for (int i = 1; i <= metaData.getColumnCount(); i++) {
                String colName = metaData.getColumnName(i);
                String colClassName = metaData.getColumnClassName(i);
                Class colClass = Class.forName(colClassName);
                Object colValue = colClass.cast(queryResults.getObject(i));

                row.put(colName, colValue);
            }
            tblData.add(row);
        }
    }

    // ...
  }

Then, I'd like to construct objects to represent news articles based on the Map<String, Object> retrieved via DbTable, like this:

public class NewsArticle {
    private final String Id, Title, Story;
    private final Timestamp Pubdate;

    public NewsArticle(Map<String, Object> row) {
        String id = null; String title = null; String story = null;
        Timestamp pubdate = null;

        for (Map.Entry<String, Object> col : row.entrySet()) {
            if (col.getKey().equals("id")) { 
                id = (String) col.getValue();
                continue;
            }
            if (col.getKey().equals("pubdate")) { 
                pubdate = (Timestamp) col.getValue();
                continue;
            }
            if (col.getKey().equals("title")) {
                title = (String) col.getValue();
                continue;
            }
            if (col.getKey().equals("story")) {
                story = (String) col.getValue();
            }
        }

        Id = id;
        Pubdate = pubdate;
        Title = title;
        Story = story;
    }

    // ...
}

So, a Map represents a database table entry as the column name, mapEntry.getKey(), and the data in the column, mapEntry.getValue().

Because I lose type safety, I'm thinking this is rubbish. But, (1) users of DbTable most certainly should know the type of each column. (2) if they do not know the type, then they can use reflection as a backup way to make DbTable usable.

This solution seems to work fine. Any opinions about losing that type safety? How could I improve this?

\$\endgroup\$
  • \$\begingroup\$ Welcome to Code Review! Is this your code ? You seems to want an explanation of how the code works. If this is what you are looking for know that this type of question is off-topic for Code Review. Please read the help center for more info. \$\endgroup\$ – Marc-Andre Oct 26 '15 at 17:04
  • \$\begingroup\$ @Marc-Andre I did write the code. As can be seen, I put 3 pieces of data into one map: "col-name", "object", and (by casting) I snuck in the object type (as would be viewable by reflection). So, I'm just thinking it is probably a bad solution because Java code should be strongly typed. And, I'm breaking that convention. But, it does work and I personally like how generic it is. So, what should I do... if it suits my needs, should I subvert Java's type safety? \$\endgroup\$ – david Oct 26 '15 at 17:44
  • \$\begingroup\$ @Marc-Andre Are classes similar to that in a textbook? I tried googling, but I can't find anything. Could I have asked the question in a different way? \$\endgroup\$ – david Oct 26 '15 at 17:57
  • \$\begingroup\$ Since it's your code and you just want a better way that preserve type-safety, your question is perfectly on-topic! I was just checking with you to make sure! I hope you have good review of your code! If you want type safety you can look into JPA. \$\endgroup\$ – Marc-Andre Oct 26 '15 at 18:02
  • \$\begingroup\$ @Marc-Andre You're almost surely correct about JPA. Why am I trying to solve problem that is so common that of course there is already a professionally done solution... my design just does not feel acceptable. \$\endgroup\$ – david Oct 26 '15 at 22:32
2
\$\begingroup\$

Coding Issues

  1. In NewsArticle, the names of the attributes begin with uppercase letters (Id, Title, Story, Pubdate). This violates common Java field naming conventions.

  2. Database column names are usually case insensitive. Depending on the underlying DBMS, the instructions like col.getKey().equals("id") may fail, because the searched value could have been stored under e.g. ID key when being read in DbTable. Either it should be normalized by applying toLowerCase() there, or it should be checked with equalsIgnoreCase() in NewsArticle constructor.

  3. Looping over the entire row map in NewsArticle constructor is unnecessary. The fields may be initialized with e.g. id = (String) row.get("id"); and there is no need to use local variables id, title etc.

  4. In DbTable: Class colClass var is not typed (raw type), which is not good. This variable is useless, because colValue is an Object and can be directly initialized with queryResults.getObject(arg), without casting.

Design Issues: Critical!

There are some very important issues concerning DbTable class.

  1. Loading the contents of an entire table using tblData Set is very dangerous. First of all, if the table is rather huge (imagine just several dozens of thousands of rows), this object will squatter the memory and probably consume too much of it uselessly. It also duplicates the data supposed to be stored in the DB and produces too much overhead on each DbTable object creation by reading all the data. However, reading just the metadata of table columns and their types could have been useful for your problem.

  2. prpStmt and queryResults objects are created, but never closed. Initializing them inside a try-with-resources block will solve the issue.

  3. throws Exception, especially in a constructor, looks very ugly. Either replace it with a more concrete exception type or (even better) find a way to avoid the throws clause for the constructor.

  4. There are no validity checks for Connection and String arguments. What will happen, for example, if one of them or both are null?

These remarks do not answer your initial question about the type safety of the solution. It's too early to answer it before fixing many of the critical issues.

\$\endgroup\$
  • \$\begingroup\$ Now that's what I'm talking bout! I knew that was garbage, I just couldn't articulate why. thanks. I'll do better next time! \$\endgroup\$ – david Nov 1 '15 at 16:48

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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