6
\$\begingroup\$

I've got a small tomcat web application that's used to receive customer data from an iPad app. After receiving those information they will be saved on a local MySQL database and then sent to an SAP system.

I'm focusing on my "concept" of handling database connections since this is my first ever productive web app that makes intense use of the database. I was wondering if there is something wrong in the way I talk with the database or if this might be prone to problems in future.

I have a class Database that creates the connection to the database and then provides a PreparedStatement through a method getPreparedStatement(String sql) that can be filled with the SQL query. It looks like this:

public class Database {
    private Connection connection = null;

    private Config config = ConfigurationUtils.config;

    public Database() {
        try {
            // Load the MySQL driver
            Class.forName("com.mysql.jdbc.Driver");

            // Create the database connection
            this.connection = DriverManager.getConnection("jdbc:mysql://" + config.getDatabase().getHost() + ":" + config.getDatabase().getPort() + "/" + config.getDatabase().getDatabaseName() + "?user=" + config.getDatabase().getUser() + "&password=" + config.getDatabase().getPassword());
        } catch (Exception e) {
            e.printStackTrace();
        }
    }

    public PreparedStatement getPreparedStatement(String sql) throws SQLException {
        return this.connection.prepareStatement(sql);
    }

    public void close() throws SQLException {
        this.connection.close();
    }
}

This Database class is used throughout the whole application (nearly every other class uses the Database class, some classes might even use them twice because multiple SQL queries have to be executed). The following example is the code of a method insertDealer() that's used to fill the database with master data about our dealerships:

    private void insertDealer(int dealer, String name1, String name2, String city, int zip) throws SQLException {
        String sql = "INSERT INTO dealer (id, name1, name2, city, zip) VALUES (?, ?, ?, ?, ?)";

        Database database = new Database();
        PreparedStatement preparedStatement = database.getPreparedStatement(sql);

        try {
            preparedStatement.setInt(1, dealer);
            preparedStatement.setString(2, name1);
            preparedStatement.setString(3, name2);
            preparedStatement.setString(4, city);
            preparedStatement.setInt(5, zip);

            preparedStatement.executeUpdate();
        } finally {
            preparedStatement.close();
            database.close();
        }
    }

Would this kind of Database access count as bad behaviour? Everything works actually like it should, I'm not experiencing any strange problems. Only thing I noticed that is when I stop my tomcat web app, I get this entry in the tomcat server log:

/webapp/statistics##1.0 appears to have started a thread named [MySQL Statement Cancellation Timer] but has failed to stop it. This is very likely to create a memory leak.
\$\endgroup\$
2
\$\begingroup\$
  1. I don't like repeated calls like this config.getDatabase().getHost() + ":" + config.getDatabase().getPort() + .... config.GetDatabase() should be called once and stored in a local variable.

  2. You will run into trouble when writing unit tests. Right now you have no way of mocking out the database access which forces you to run against a real MySql database which tends to make unit testing a pain in the backside.

    Create an interface for Database against which all your business logic should work and inject it into the classes responsible for creating the database queries. Then during unit testing you can pass in a mock-implementation which simply checks that the methods are preparing the statements correctly without having to run a query against an actual database.

\$\endgroup\$
1
  • \$\begingroup\$ 1: I think I'll have to overthink the naming of my config classes, because getDatabase() returns an object called Database also. 2: Thanks for that :) \$\endgroup\$ – Ahatius Feb 27 '15 at 5:14
2
\$\begingroup\$

There are many problems with the constructor of the Database class:

  • The driver class name and connection URL are hardcoded. So you have a class with a very general name, but tied to one very specific database.

  • If something goes wrong during construction (driver class not found, connection errors), the program will print a stack trace on the console. And since the exception is swallowed, the caller will happily continue with his business, not knowing that the Database instance is defunct.

Improvement ideas:

  • Give the constructor parameters: driver name, connection string

  • Don't swallow the exception. Log the details and re-throw, probably a custom exception will make more sense to encapsulate the details.

  • Using a simple connection string like jdbc:mysql://servername:1234/dbname?user=alice&password=secret will be more ergonomic than piecing it together from multiple parameters

You mention that you use this class everywhere, and gave an example of a method where you create an instance of Database:

private void insertDealer(int dealer, String name1, String name2, String city, int zip) throws SQLException {
    String sql = "INSERT INTO dealer (id, name1, name2, city, zip) VALUES (?, ?, ?, ?, ?)";

    Database database = new Database();
    // ...

If this is the case, then you're creating multiple Connection objects needlessly, and not reusing Database instances well. It would be better to create one instance of Database early in the program, and pass it to every component that needs it.

\$\endgroup\$
1
  • \$\begingroup\$ I guess I could outsource the driver and complete connection string to the configuration file - that way I get some flexibility without having to change all class calls. Regarding the connection object: Are you suggesting that I should have a Connection that's opened once the webapp is started or that I should have one Connection for each request made (whereas some servlets use the Database object up to 3 times)? \$\endgroup\$ – Ahatius Feb 27 '15 at 5:28

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.