NOTE: I was given permission to post the codes here.
I am in my internship currently. We are maintaining a system that is quite used in the organization. It is written in Java. I am assigned to refactoring the code and I chose to work on the database module, thinking it was just an easy job to do.
When I saw the code, it is full of singletons and I feel that the module could be done in a better way. One of the classes in the module is InsertDatabase. It handles every data insertion. It looks like this:
public class InsertDatabase {
private static InsertDatabase instance = null;
private Properties prop = new Properties();
private InputStream input = null;
private InsertDatabase(){
try{
input = new FileInputStream("textfile/sql_queries.txt");
prop.load(input);
}catch(Exception e){
System.err.println(e);
}
}
public static InsertDatabase getInstance(){
if(instance == null) {
instance = new InsertDatabase();
}
return instance;
}
public void insertIntoCourseTable(String course_abbr, String course_name) {
Connection conn = cpInstance.getConnection();
try(PreparedStatement ps = conn.prepareStatement(prop.getProperty("insert_course_table"));) {
ps.setString(1, course_abbr);
ps.setString(2, course_name);
ps.executeUpdate();
Helper.getInstance().aprintln(this,Helper.LOG_VERBOSE,"");
Helper.getInstance().aprintln(this,Helper.LOG_VERBOSE,"*** Information Added into Course Table***");
} catch (SQLException e) {
Helper.getInstance().aprintln(this,Helper.LOG_VERBOSE,"Data already exists");
}finally {
try {
conn.close();
} catch (SQLException e) {
e.printStackTrace();
}
}
// more code...
}
This is also the same case with the other classes assigned to table creation, updating data, and selecting data.
The SQL queries are actually stored in a text file called sql_queries.txt and is loaded in the variable props. The text file is almost 200 lines long. Here's a sample line from the text file.
insert_diagnostic_table = insert into diagnostic_table (date_start, date_end, sem_num, freetime, time_penalty, balance_penalty) values (?, ?, ?, ?, ?, ?)
insert_pasa_balance_table = insert into pasa_balance_table (date_rendered, sender, amount, current_balance_sender, deducted_balance_sender, receiver, current_balance_receiver, added_balance_receiver) values (curdate(), ?, ?, ?, ?, ?, ?, ?)
The classes are then herded into a singleton class called Information, which the system uses to interact with the database. The class looks like this:
public class Information {
private static Information instance = null;
private InsertDatabase insert_db = InsertDatabase.getInstance();
private SearchDatabase search_db = SearchDatabase.getInstance();
private UpdateDatabase update_db = UpdateDatabase.getInstance();
private DeleteDatabase delete_db = DeleteDatabase.getInstance();
private TransDatabase trans_db = TransDatabase.getInstance();
public static Information getInstance(){
if(instance == null) {
instance = new Information();
}
return instance;
}
/*
* Here lies all of the methods that access DeleteDatabase
*/
public void storeToDeleteReservation(String username) {
delete_db.deleteReservation(username);
}
// more code...
public void storeStudentToDelete(String username) {
delete_db.deleteStudent(username);
}
/**
* Deletes Staff
* @param username
*/
public void storeStaffToDelete(String username) {
delete_db.deleteStaff(username);
}
/*
* End of methods that access DeleteDatabase
*/
/*
* Here lies all the methods that access InsertDatabase
*/
public void storeAccountInfo(String username, String password, String last_name, String first_name, String mid_name, String suffix_name){
insert_db.insertIntoAccountTable(username, password, last_name, first_name, mid_name, suffix_name);
}
public void storeCourseInfo(String course_abbr, String course_name) {
insert_db.insertIntoCourseTable(course_abbr, course_name);
}
// more code...
}
I have a weird feeling that there are problems with this. I can't pinpoint exactly what the problems are. Having them as singletons is a warning flag for me already. So, are there any problems with the approach?
What I was thinking to do was to create a DAO (data access object) for each table and have the rest of the system use the DAOs instead. If I proceed with DAOs, I'd modify Information first so that I would not touch the rest of the system and accidentally break anything. If I do break anything, at least I know where I screwed up. What do you think?
Thanks! :)
Top comments (7)
Why?
Add some unit/integration tests instead. Given tests will be implemented properly, you'll have a guarantee that if anything breaks, your module is not the culprit.
Singletons tend to be misused so my warning flag goes up.
I'll be doing that in a bit.
Singleton is just a way to ensure that only one instance of some class is present at runtime. If the project you're working on has DI set up, then you probably can get rid of them without much trouble.
Since there is a text file with queries, maybe try JDBI. It's not exactly an ORM, but from the SQL code you posted I've got an impression that you may do fine without one.
I do am considering JDBI and utilize the data access object pattern. Thanks!
Several, but the god class being a singleton in this case is not one of them. As a thought experiment, imagine someone would edit that property file and the system would read the file in at runtime in different states, so you'd be up to some possibly very dynamic behaviour (mind you, not in the good sense of the word). Luckily, that seems not to be the case. Externalizing the statements doesn't make a lot of sense, and I think inlining them would be quite appropriate, that seems not to be the main pain point.
From the snippets you gave, even without knowing the exact usage of the Information class it is very clear, that this class doesn't provide any meaningful abstraction in itself (really, which parts of any software system are NOT in some sense "Information") and exposes a much too large API surface to the classes which are using it, a text book example for a violation of the interface segregation principle actually. Therefore an important step would be to identify the several interfaces that are hidden in your twohundred-something methods, extract them, make the Information class implement them and make the current users of Information gradually independend from it. I wouldn't touch Information further, except for adding which of the newly identified and created interfaces it already implements. When you have the interfaces, you will see if it actually were DAOs in disguise or if the underlying design was more of a row data gateway or an active record. Be it as it may, when you have the interfaces extracted reimplement them and consider this as an opportunity to re-inline the queries. And when you are done with this, the names of the methods would be a worthy aspect on which some to spend some time working on.
Good luck with cleaning the augean stables ;)
I would look to using an ORM. Essentially you could create your entities and models from your database schema. This first step is fairly easy to do with most ORM's. I have done this many times with Hibernate. Focus on keeping things simple. Do not map your ER relationships (foreign keys). The reason not to map your foreign keys in your ORM is that you will very likely want to control the fetching and storing of your data on a finer-grained scale unless your database is relatively small and fast. One caveat here is that some ORMS require you to map the foreign keys (ie. they do not provide a means not to). Don't pick these ORMs.
Do not focus on performance at first. Keep a list of the tables / objects that are frequently read and seldom written.
There are many ways to generate your POJOS, here are a couple to get you started. Do some looking around and find one to your liking. I personally like the fluent style so I have listed these first (I have not used these):
vladmihalcea.com/fluent-api-entity...
github.com/v-ladynev/fluent-hibernate
mkyong.com/hibernate/how-to-genera...
eclipse.org/webtools/dali/docs/3.2...
Once you have generated your mappings you will still have another class (or classes) to hold some custom db logic (including that logic that makes up for not mapping the foreign keys). This will serve as the glue layer between your app(s) and your ORM.
Lastly, you will have a singleton class that will handle reading the configuration and connecting / disconnecting from your DB. Here is where I inject my encrypted passwords read from a secured auth source as well.
Lastly you will tune for performance. Here is where you will provide some stateless sessions for batch reading of those tables that were frequently read and seldom written as well as for anything that is written and seldom read (g.g. logs). Here you may also need to judiciously introduce some locking.
Depending on the size of your DB and your familiarity with ORMs these steps will take at least one and likely several of those mythical man-months.
I'd take issue with the db statements being isolated, can't insert and update in the same transaction (unless I'm missing something).
I would recommend against using a standard repository pattern in the case you migrate to an ORM since a repo pattern layer tends to have adverse effects on the ORMs performance and the additional abstraction doesn't add any benefit. But I guess that would depend on the ORM.
If a web application, I like creating a connection instance per request.