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?
-
\$\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– Marc-Andre2015年10月26日 17:04:25 +00:00Commented Oct 26, 2015 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.t– david.t2015年10月26日 17:44:11 +00:00Commented Oct 26, 2015 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.t– david.t2015年10月26日 17:57:24 +00:00Commented Oct 26, 2015 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– Marc-Andre2015年10月26日 18:02:21 +00:00Commented Oct 26, 2015 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.t– david.t2015年10月26日 22:32:02 +00:00Commented Oct 26, 2015 at 22:32
1 Answer 1
Coding Issues
In
NewsArticle
, the names of the attributes begin with uppercase letters (Id, Title, Story, Pubdate
). This violates common Java field naming conventions.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 inDbTable
. Either it should be normalized by applyingtoLowerCase()
there, or it should be checked withequalsIgnoreCase()
inNewsArticle
constructor.Looping over the entire
row
map inNewsArticle
constructor is unnecessary. The fields may be initialized with e.g.id = (String) row.get("id");
and there is no need to use local variablesid
,title
etc.In
DbTable
:Class colClass
var is not typed (raw type), which is not good. This variable is useless, becausecolValue
is anObject
and can be directly initialized withqueryResults.getObject(arg)
, without casting.
Design Issues: Critical!
There are some very important issues concerning DbTable
class.
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 eachDbTable
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.prpStmt
andqueryResults
objects are created, but never closed. Initializing them inside a try-with-resources block will solve the issue.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 thethrows
clause for the constructor.There are no validity checks for
Connection
andString
arguments. What will happen, for example, if one of them or both arenull
?
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.
-
\$\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.t– david.t2015年11月01日 16:48:21 +00:00Commented Nov 1, 2015 at 16:48
Explore related questions
See similar questions with these tags.