Database abstraction in Java

Question

I am working on a project in Java which includes a lot of database related tasks. For instance, I would have to insert, update, retrieve data so many times within the application. To use code reusability, I've just written this abstraction for my database class. I'm planing to call its methods where ever its needed to perform database related tasks within the code. Please review my code. Are there any bad practices?

import java.sql.Connection;
import java.sql.DriverManager;
import java.sql.PreparedStatement;
import java.sql.ResultSet;
import java.sql.SQLException;
import java.sql.Statement;

import com.teamincredibles.settings.Settings;

public class Database {

    private Connection connection;

    public void connect() {
        try {
            Class.forName(Settings.getDatabaseDriver());
            connection = DriverManager.getConnection(Settings.getJdbcUrl(), Settings.getDBUserName(), Settings.getDBPassword());
        } catch (Exception e) {
            e.printStackTrace();
        }
    }

    public boolean prepareStatement(String query, String choice1) {
        boolean flag = false;
        try {
            PreparedStatement ps = connection.prepareStatement(query);
            ps.setString(1, choice1);
            ResultSet rs = ps.executeQuery();
            flag = rs.next();
        } catch (SQLException e) {
            e.printStackTrace();
        }
        return flag;
    }

    public boolean prepareStatement(String query, String choice1, String choice2) {
        boolean flag = false;
        try {
            PreparedStatement ps = connection.prepareStatement(query);
            ps.setString(1, choice1);
            ps.setString(2, choice2);
            ResultSet rs = ps.executeQuery();
            flag = rs.next();
        } catch (SQLException e) {
            e.printStackTrace();
        }
        return flag;
    }

    public void executeUpdate(String query) {
        try {
            Statement statement = connection.createStatement();
            statement.executeUpdate(query);
        } catch (SQLException e) {
            e.printStackTrace();
        }
    }

    public boolean execute(String query) {
        boolean result = false;
        try {
            Statement statement = connection.createStatement();
            result = statement.execute(query);
        } catch (SQLException e) {
            e.printStackTrace();
        }
        return result;
    }

    public ResultSet executeQuery(String query) {
        ResultSet result = null;
        try {
            Statement statement = connection.createStatement();
            result = statement.executeQuery(query);
        } catch (SQLException e) {
            e.printStackTrace();
        }
        return result;
    }

    public void disconnect() {
        try {
            if (connection != null) {
                connection.close();
            }
        } catch (Exception e) {
            e.printStackTrace();
        }
    }
}

Answer 1

Code Smells

Remember the DRY and KISS principles of programming.

Here are some other resources with good info on general programming principles.

• 3 Key Software Principles You Must Understand
• The Principles of Good Programming

As a maintainer or reviewer of anyone's code (our own or someone elses), when examining code we look for code smells. These give general indications of programming practices which are less than ideal. When we find code smells, it's a good clue that we need to look more closely and consider why the code smells.

There are a few obvious code smells, mostly pointed out by others already:

• Ambiguous Naming Convention
• Re-purposing Existing Names
• Repetitive Code Patterns
• Improper Use of Exceptions
• Failure to Release Resources
• Lack of Comments

Naming Conventions

Objects and other Variables are nouns. Methods and other functions are verbs. Name them accordingly. Do not use acronyms or abbreviations, but do not use sentences for your names either. There is no need for joining words such as 'The' or 'With'. Most importantly, one should also always strive to maintain consistent naming convention, whatever style you choose. This is more in reference to @Shree's use of executeWithPreparedStatement.

executePreparedStatement is better. It conforms to the naming convention used for the other methods in the library and avoids the use of an unnecessary preposition that doesn't add any meaning to the identifier.

Same for DBUtil. These abbreviations are common enough that there is little risk of confusion but it should still be avoided. DatabaseUtility is best. We can only assume that DB means Database, we can't be sure until we inspect source code or documentation. Forcing the reader of the code to have to guess, or research should be avoided whenever possible.

Re-using names that you are deriving from is OK when you are recreating a function that does the same thing in a different way. It is not OK when you have extended the functionality of the code you are deriving from. Pick a new name that better describes why your method is different than the original.

Repetitive Code Patterns

Next to KISS, DRY should be the most important principle of programming. DRY is why we do programming. To automate things. Failing the DRY principle means you are doing more work than you need to, introducing more potential for error and making it more difficult to maintain.

When you see repetitive code patterns, such as blanket exception handling or methods that look almost identical, consider why is it repetitive and how can you reduce the duplication of those pieces of code. Often it means reconsidering the method of implementation, not just splitting things up more.

Exception Handling

Errors and exceptions should always be handled gracefully.

Never trap exceptions and then not handle them. Printing a stack trace is not handling them. You are merely acknowledging that a problem can happen at that point but you don't care. Why add extra code if you don't care? The only thing accomplished is that the problem is guaranteed to not be fixable from outside of your function, since no caller can know that anything went wrong.

Exceptions that are unhandled should be allowed to flow up the call stack until they are handled, or until the main function is reached and then an unhandled exception error occurs.

You are better off to simply not catch the exceptions at all and just let them get passed on until you get an unhandled exception error.

You can trap them do something with them and then rethrow the exceptions if they are not actually being handled.

Resource Leaks

Make sure that you are not overriding valid handles when you open new connections. The connect() method needs to make sure that the connection object is not already valid.

As well, in the disconnect() method you should reassign connection=null after it has been closed so that the invalid handle can't accidentally be used.

Make sure to close all open handles when you are done with them. This includes things like PreparedStatement, Statement and ResultSet objects, not just connections. These objects go into object pools just like the connection pool, which can build up and eat RAM if you forget to close things. (TIP: they all derive from the AutoCloseable/Closeable interface)

Lack of Comments

This seems so trivial but it is not. This is the biggest code smell. While we should always strive to create self-documented code, we still need comments to summarize what it is the code is doing and so that formal documentation can be generated with the least effort, written by the people who actually wrote the code. Comments come first, before code. Always. This is one most programmers most often neglect. Me too.

I should mention that I'm mostly speaking of inline documentation, not trying to teach the reader how every bit of code works.

The converse code smell to a lack of comments is too much comments. If you need comments to describe how your code works and not just what it does, then your code is probably too complex.

Answer 2

Most of the review comments are covered and all are valid comments. apart from them it would be worth to look at below comments

Code smells 1) code duplications : a lot of places code is duplicated . You have added two preparedStatement methods just to add one extra parameter. will you add third method if you need third choice ?

I refactored your code as you can find that executeWithPreparedStatement() can be sufficient for your need .

2)Please work on naming conventions :

Class Name Database is very generic .please use either DatabaseOperations or simply DBUtil would be fine.

Method name prepareStatement doesnt give me any indication what this method would do ? I will prefer executeWithPreparedStatement

Resource Leak

Resources are expensive and you should close them immediatly your work is over and to make sure that they are getting closed you should close them in finally block.

Modification suggested

1) I believe that as to call every other method of DBUtil you need connection. hence it is primary responsibility that we should call connect method in constructor method. Imagine a scenario where you called executeWithPreparedStatement() method before calling connect() you would get NullPointer Exception

2)you should add extra method to check if connection is closed already so you can write your safety net

3)Usually returning Map records in executeWithPreparedStatement () method would be useful if you want to read data.

import java.sql.*;
import java.util.HashMap;
import java.util.Map;

import com.teamincredibles.settings.Settings;

public class DBUtil {
    private PreparedStatement prepStatement = null;
    private ResultSet resultSet = null;
    private Connection connection = null;
    private Statement statement=null;

    public DBUtil() throws SQLException, ClassNotFoundException {
        connect();
    }

    public boolean isConnectionClosed() throws SQLException {
        return null==connection || connection.isClosed();
    }

    public void connect() throws ClassNotFoundException, SQLException {
            Class.forName(Settings.getDatabaseDriver());
            connection = DriverManager.getConnection(Settings.getJdbcUrl(), Settings.getDBUserName(), Settings.getDBPassword());
    }


    public Map<String,String> executeWithPreparedStatement(String query, String ...values) throws SQLException {
        Map<String,String> records = new HashMap<String,String>();
        boolean flag = false;
        try {
            prepStatement = connection.prepareStatement(query);
            int noOfParameters = values.length;
            for(int i=0;i<noOfParameters;i++) {
                prepStatement.setString(1, values[i]);
            }
            resultSet = prepStatement.executeQuery();
            ResultSetMetaData resultSetMetaData = resultSet.getMetaData();
            while(resultSet.next()){
                    for(int i=1;i<=resultSetMetaData.getColumnCount();i++){
                        records.put(resultSetMetaData.getColumnName(i),resultSet.getString(resultSetMetaData.getColumnName(i)));
                    }
            }

        }  finally {
            releaseResources();
        }
        return records;
    }

    private void releaseResources() throws SQLException {
        if(null!=resultSet){
            resultSet.close();
        }
        if(null!=prepStatement){
            prepStatement.close();
        }
        if(null!=statement){
            statement.close();
        }
    }

    public int executeUpdate(String query) throws SQLException {
            try {
                statement = connection.createStatement();
                return statement.executeUpdate(query);
            }finally {
                releaseResources();
            }
    }

    public boolean executeDDL(String query) throws SQLException {
        try {
             statement = connection.createStatement();
            return statement.execute(query);
        }finally {
            releaseResources();
        }
    }

    public void disconnect() throws SQLException {
            if (connection != null && !connection.isClosed() ) {
                connection.close();
            }
    }
}

Important tips:

1) Always close the resources after your use is over

2) handle exceptions gracefully.

3) try to re-factor the code.

Answer 3

1. Memory leaks. note that your connection is never closed, and thats a memory leak, you could make your class Closeable and close the connection there.

   class Database implements Closeable{
     private Connection connection;
     ...
     public void close() throws Exception{
       if(this.connection!=null){
          this.connection.close();
        }
     }
   }
   

2. You don't need to load the driver every time you instantiate the class, you could load it once using a static block

   static{
     Class.forName(Settings.getDatabaseDriver());
   }
   

3. I wouldn't instantiate connections that way, use Datasources instead.

4. Database isn't a good name for such a utility class, DBUtility would be better perhaps.

Answer 4

Sorry, but this is just all wrong. If printing a stack trace was better than throwing an exception, the java.sql methods would do it. If creating a Statement for each query was a good idea, they'd do it.

What you did is what I did once (when the coal was young) too: By trying to improve things you made them worse. You're right that checked exceptions are a pain, but you're wrong thinking that you must handle them immediately. Most of the time, writing throws SQLException is all you have to do.

---

public boolean prepareStatement(String query, String choice1) {
    ...
    ResultSet rs = ps.executeQuery();
    ...
}

This is at best misnamed, as not every statement has a result set (it can be an update or insert). Moreover, you're opening it only to see if it's empty... and later you probably open it again to read it. This means doubling the overhead. As already said, you're leaking both the PreparedStatement and the ResultSet.

---

Concerning the commit/rollback idea by @Douglas, it's wrong, too. You do not want to commit after every single SQL command as the command may not be a complete piece of work may put the DB in an inconsistent state. Imagine transferring money from one account to another. You surely don't want to commit after changing the first balance!

In case committing every statement is fine, you can use autocommit.

Answer 5

I am working on a project in Java which includes a lot of database related tasks

At the risk of being off topic, rather than improving your code, you should be looking to use a tool to help you access the database. Some suggestions:

• Spring JDBC templates
• Hibernate
• myBatis (my personal favourite)

The advantage of using a tool is that getting the "boilerplate" code correct is no longer a problem. Transaction handling is usually done very well by these frameworks, and is almost invisible to the programmer. Adding new queries is simple and as the boilerplate is done for you, and is more likely to be error free.

The disadvantage is that there is a definite learning curve. But in my experience, once you master the tool, you won't go back.