patternjavaMinor
Constructing NewsArticle objects from a database table
Viewed 0 times
objectsdatabaseconstructingnewsarticlefromtable
Problem
This class seems to be able to represent the data stored in any possible database table:
private final String Id, Title, Story;
private final Timestamp Pubdate;
public NewsArticle(Map row) {
String id = null; String title = null; String story = null;
Timestamp pubdate = null;
for (Map.Entry 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;
}
// ...
}
This
public class DbTable {
private Set> tblData = new HashSet>();
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 row = new HashMap();
for (int i = 1; i
Then, I'd like to construct objects to represent news articles based on the Map retrieved via DbTable, like this:
public class NewsArticle {private final String Id, Title, Story;
private final Timestamp Pubdate;
public NewsArticle(Map row) {
String id = null; String title = null; String story = null;
Timestamp pubdate = null;
for (Map.Entry 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
Coding Issues
-
In
-
Database column names are usually case insensitive. Depending on the underlying DBMS, the instructions like
-
Looping over the entire
-
In
Design Issues: Critical!
There are some very important issues concerning
-
Loading the contents of an entire table using
-
-
-
There are no validity checks for
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.
-
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 in DbTable. Either it should be normalized by applying toLowerCase() there, or it should be checked with equalsIgnoreCase() in NewsArticle constructor.-
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.-
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.-
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.-
prpStmt and queryResults 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 the throws clause for the constructor.-
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.
Context
StackExchange Code Review Q#108796, answer score: 2
Revisions (0)
No revisions yet.