12
\$\begingroup\$

I've recently learned how to use MySQL to store/retrieve data. I've created a MySQLManager class for creating connections and executing queries/updates. Would this be an efficient way to do so?

public class MySQLManager {

private String host;
private String port;
private String database;
private String user;
private String password;

public MySQLManager(String host, String port, String database, String user, String password){
    this.host = host;
    this.port = port;
    this.database = database;
    this.user = user;
    this.password = password;
}

private Connection createConnection(){
    Connection connection;
    try {
        Class.forName("com.mysql.jdbc.Driver");
        connection = DriverManager.getConnection("jdbc:mysql://" + host + ":" + port + "/" + database, user, password);
        return connection;
    } catch (SQLException e) {
        System.out.println("Could not connect to MySQL server! because: " + e.getMessage());
    } catch (ClassNotFoundException e) {
        System.out.println("JDBC Driver not found!");
    }
    return null;
}

public ResultSet executeQuery(String query){
    Connection connection = createConnection();
    Statement statement = null;
    ResultSet set = null;
    try {
        statement = connection.createStatement();
        set = statement.executeQuery(query);
        return set;
    } catch (SQLException e) {
        e.printStackTrace();
    } finally { //Close in order: ResultSet, Statement, Connection.
        try { set.close(); } catch (Exception e) { }
        try { statement.close(); } catch (Exception e) { }
        try { connection.close(); } catch (Exception e) { }
    }
    return null;
}

public void executeUpdate(String update){
    Connection connection = createConnection();
    Statement statement = null;
    try {
        statement = connection.createStatement();
        statement.executeUpdate(update);
    } catch (SQLException e) {
        e.printStackTrace();
    } finally {
        try { statement.close(); } catch (Exception e) { }
        try { connection.close(); } catch (Exception e) { }
    }
}
}

I would then use this to execute queries/updates:

MySQLManager manager = new MySQLManager(host, port, database, user, pass);
manager.executeQuary("SELECT * FROM table;");
manager.executeUpdate("Some update;");

Are there any better ways of doing this? Is there anything I can improve in my code?

\$\endgroup\$
1
  • 4
    \$\begingroup\$ Do you have a good reason for reconnecting to the DB for every query? Why not just use one connection for each instance of your manager class? \$\endgroup\$ Jun 27 '14 at 9:35
7
\$\begingroup\$

You've written a wrapper around JDBC that automatically connects/disconnects, and swallows exceptions. Don't do that.

JDBC wants you to establish connections explicitly because it's a high-overhead activity. Connecting involves a TCP handshake, authentication, opening the requested database, and some work to set up a session (which includes some locale settings, for example). You have just taken a good design decision of JDBC (and all other database APIs) and made it worse.

Furthermore, if every query/update is in its own connection, then you can't execute multiple statements in a transaction. (MySQL users often don't appreciate the value of transactions.)

Finally, SQLException exists for a reason: they allow your code to gracefully handle failures. You merely swallow the exceptions after printing a stack trace. You might as well be writing your data to /dev/null if you don't care whether your writes succeeded. Also, returning null instead of propagating an exception does not make your caller's code any simpler.

I strongly recommend discarding your entire MySQLManager class and using JDBC as is.

\$\endgroup\$
5
\$\begingroup\$

Following example of Spring JdbcTemplate, I often use this approach to write my persistence layer:

You could write only one DAO class and reuse it:

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 java.util.ArrayList;
import java.util.List;

public class JdbcDao<T> {

    private String url;
    private String username;
    private String password;
    private Connection connection = null;

    public JdbcDao(String url, String username, String password) {
        this.url = url;
        this.username = username;
        this.password = password;
    }

    public Connection getConnection() {

        try {
            Class.forName("com.mysql.jdbc.Driver");
            connection = DriverManager.getConnection(url, username, password);
        } catch (Exception exception) {
            exception.printStackTrace();
        }
        return connection;
    }

    public void createTable(String sqlStatement) {

        Statement statement = null;
        Connection connection = null;

        try {
            connection = getConnection();
            statement = connection.createStatement();
            statement.executeQuery(sqlStatement);
            System.out.println("JdbcDao info: " + sqlStatement);
        } catch (Exception e) {
            e.printStackTrace();
        } finally {
            try {
                connection.close();
            } catch (SQLException e) {
                e.printStackTrace();
            }
            try {
                statement.close();
            } catch (SQLException e) {
                e.printStackTrace();
            }
        }
    }

    public void saveOrUpdate(String sqlStatement, Object[] args) {
        Connection connection = null;
        PreparedStatement preparedStatement = null;

        try {
            connection = getConnection();
            preparedStatement = connection.prepareStatement(sqlStatement);
            for (int i = 0; i < args.length; i++) {
                preparedStatement.setObject(i + 1, args[i]);
            }
            preparedStatement.executeUpdate();
            System.out.println("JdbcDao info: " + sqlStatement);
        } catch (Exception e) {
            e.printStackTrace();
        } finally {
            try {
                connection.close();
            } catch (SQLException e) {
                e.printStackTrace();
            }
            try {
                preparedStatement.close();
            } catch (SQLException e) {
                e.printStackTrace();
            }
        }

    }

    public List<T> getAll(String sqlStatement,
            ResultSetProcessor<T> resultSetProcessor) {
        return getAll(sqlStatement, resultSetProcessor, new Object[] {});
    }

    public List<T> getAll(String sqlStatement,
            ResultSetProcessor<T> resultSetProcessor, Object[] args) {

        List<T> list = new ArrayList<T>();
        Connection connection = null;
        PreparedStatement preparedStatement = null;
        ResultSet resultSet = null;

        try {
            connection = getConnection();
            preparedStatement = connection.prepareStatement(sqlStatement);
            for (int i = 0; i < args.length; i++) {
                preparedStatement.setObject(i + 1, args[i]);
            }
            resultSet = preparedStatement.executeQuery();
            while (resultSet.next()) {
                list.add(resultSetProcessor.getProcessedObject(resultSet));
            }
            System.out.println("JdbcDao info: " + sqlStatement);

        } catch (Exception e) {
            e.printStackTrace();
        } finally {
            try {
                connection.close();
            } catch (SQLException e) {
                e.printStackTrace();
            }
            try {
                preparedStatement.close();
            } catch (SQLException e) {
                e.printStackTrace();
            }
            try {
                resultSet.close();
            } catch (SQLException e) {
                e.printStackTrace();
            }
        }

        return list;
    }

    public void deleteAll(String sqlStatement) {
        delete(sqlStatement, new Object[] {});
    }

    public void delete(String sqlStatement, Object[] args) {
        Connection connection = null;
        PreparedStatement preparedStatement = null;

        try {
            connection = getConnection();
            preparedStatement = connection.prepareStatement(sqlStatement);
            for (int i = 0; i < args.length; i++) {
                preparedStatement.setObject(i + 1, args[i]);
            }
            preparedStatement.executeUpdate();
            System.out.println("JdbcDao info: " + sqlStatement);
        } catch (Exception e) {
            e.printStackTrace();
        } finally {
            try {
                connection.close();
            } catch (SQLException e) {
                e.printStackTrace();
            }
            try {
                preparedStatement.close();
            } catch (SQLException e) {
                e.printStackTrace();
            }
        }
    }
}

Then you could write some ResultSet mapper interface:

import java.sql.ResultSet;
import java.sql.SQLException;

public interface ResultSetProcessor<T> {
    T getProcessedObject(ResultSet rs) throws SQLException;
}

And this would be basic usage:

import java.util.List;

import org.jdbcfactory.JdbcDao;

public class TestDao {
    // Usage of JdbcDao
    public static void main(String[] args) {

        JdbcDao<Student> jdbcDataAccess = new JdbcDao<Student>(
                "jdbc:mysql://localhost:3306/db", "username", "password");

        // Create table
        jdbcDataAccess
                .createTable("create table if not exists student "
                        + "(id int primary key auto_increment, name varchar(45), age int)");


        // Insert new record
        jdbcDataAccess.saveOrUpdate(
                "insert into student (name, age) values (?, ?)", new Object[] {
                        "John Doe", 25 });

        // Update
        jdbcDataAccess.saveOrUpdate(
                "update student set name = ?, age = ? where id = ?",
                new Object[] { "Jane Doe", 21, 1 });

        // Delete
        jdbcDataAccess.delete("delete from student where id = ?",
                new Object[] { 1 });

        // Select all
        List<Student> list = jdbcDataAccess.getAll("SELECT * FROM student",
                rs -> {
                    Student student = new Student();
                    student.setId(rs.getInt("id"));
                    student.setName(rs.getString("name"));
                    student.setAge(rs.getInt("age"));

                    return student;
                });

        // Select single
        List<Student> list2 = jdbcDataAccess.getAll(
                "select * from student where id = ?", rs -> {
                    Student student = new Student();
                    student.setId(rs.getInt("id"));
                    student.setName(rs.getString("name"));
                    student.setAge(rs.getInt("age"));

                    return student;
                }, new Object[] { 1 });

        list.forEach(student -> System.out.println(student.getId() + " :: "
                + student.getName() + " :: " + student.getAge()));

        list2.forEach(student -> System.out.println(student.getId() + " :: "
                + student.getName() + " :: " + student.getAge()));

    }

    public static class Student {

        private int id;
        private String name;
        private int age;

        public int getId() {
            return id;
        }

        public void setId(int id) {
            this.id = id;
        }

        public String getName() {
            return name;
        }

        public void setName(String name) {
            this.name = name;
        }

        public int getAge() {
            return age;
        }

        public void setAge(int age) {
            this.age = age;
        }

    }

}

Advantage of this approach is that you don't have to always open connections, create statements, execute them, close them... You just create one DAO class and use it as shown above. You could also create separated class with static method which will return connection instead of creating one connection instance per JdbcDao instance. Also, in my example of usage, I passed lambda expression instead anonymous inner class of ResultSetProcessor. You could create separated class which will implement ResultSetProcessor interface instead of passing lambda. Why did I choose this approach? Although it's a bit verbose in some cases, it minimizes redundancy of code. Only thing you should create are POJO's (with properties which reflect tables in database) and implementation classes for ResultSetProcessor interface. It's not perfect, but it's enough for basic needs.

\$\endgroup\$
2
  • 1
    \$\begingroup\$ While this could be useful as code, we don't really like to throw code at the OP here at Code Review. We are here to help the op getting better, could you maybe explain why your code could be better versus what he already have ? \$\endgroup\$
    – Marc-Andre
    Jun 30 '14 at 1:07
  • \$\begingroup\$ @Marc-Andre Done. \$\endgroup\$ Jun 30 '14 at 14:08
5
\$\begingroup\$

You could use a Pooling Data Source to limit the number of opened connections.

   InitialContext ctx = new InitialContext();
   DataSource ds =(DataSource)ctx.lookup("java:comp/env/jdbc/MySQLDB");
   Connection connection = null;
   try {
        connection = ds.getConnection();
       //...
   }finally{
    try{
     if(null != connection){
     connection.close();
     }
    }catch(Exception ex){ // use a more specific exception
      // logging
    }
   }

Moreover, use Prepared Statement instead of Statements, it gives automatic prevention of SQL injection attacks.

Use proper logging instead of printStackTrace

\$\endgroup\$

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.