Efficient use of MySQL Connections with Java

Question

Background: I'm building a dynamic in-house on-call roster that will be displayed on a web page. The end user will be displayed a list of people who are currently (at the time of the database call) on-call for various organizations, departments within the organization, and offices within the department.

The basic rules are that there will be 1-n organizations to be displayed. An organization can contain 0-n departments and departments can contain 0-n offices. Organizations and department may or may not have a person on call for the entire org/dept, but an office will always have a person on call (understanding the limitations of people who might forget to designate someone on call). Organizations, departments, and offices can be added, edited, reorganized, and deleted as needs change.

The Issue: In the past, I've worked under the "one-connection" rule: You open a connection, you use it for a specific task, then you close it..not having more than one connection open at any given time. However, this causes more processing by having to loop through retrieved objects to get their subordinate data. I'd like to be more efficient, and this is my first try at it by eliminating unnecessary looping.

package oncall.database;

import java.sql.Connection;
import java.sql.DriverManager;
import java.sql.ResultSet;
import java.sql.SQLException;
import java.sql.Statement;
import java.text.SimpleDateFormat;
import java.util.ArrayList;
import java.util.Date;
import java.util.List;
import data.Department;
import data.Office;
import data.OnCall;
import data.Organization;

public class DatabaseUtil {

    private enum OrgLevel { // for use in getOnCall database call

        ORG("organization"),
        DEPT("department"),
        OFF("office");

        private final String orgLevel;

        private OrgLevel(final String s) {
            orgLevel = s;
        }

        public String getOrgLevel() {
            return orgLevel;
        }
    }

    public DatabaseUtil() {

    }

    public static Connection getConnection() throws ClassNotFoundException, SQLException {
        String connectionURL = "jdbc:mysql://<server ip>:3306/oncall?user=<user>&password=<password>";  //<> = redacted
        Class.forName("com.mysql.jdbc.Driver");
        Connection conn = DriverManager.getConnection(connectionURL);
        return conn;
    }

    public static List<Organization> getOrganizations() throws ClassNotFoundException, SQLException {
        List<Organization> returnList = new ArrayList();
        Connection conn = getConnection();
        String query = "Select id, name from organization;";

        Statement s = conn.createStatement();
        ResultSet rs = s.executeQuery(query);

        while (rs.next()) {
            Organization o = new Organization();
            int idInt = rs.getInt("id");
            o.setId(idInt);
            o.setName(rs.getString("name"));
            o.setDepartmentList(getDepartments(o));  //get all departments under that organization
            o.setOnCall(getOnCall(OrgLevel.ORG, idInt));  //person on call for the whole organization, may be null
            returnList.add(o);
        }
        rs.close();
        s.close();
        conn.close();

        return returnList;
    }

    public static List<Department> getDepartments(Organization o) throws ClassNotFoundException, SQLException {
        List<Department> returnList = new ArrayList<>();
        Connection conn = getConnection();
        String query = "select id, name from department where organization=" + o.getId() + ";";
        Statement s = conn.createStatement();
        ResultSet rs = s.executeQuery(query);

        while (rs.next()) {
            Department d = new Department();
            d.setId(rs.getInt("id"));
            d.setName(rs.getString("name"));
            d.setOfficeList(getOffices(d)); // get all offices in department
            d.setOnCall(getOnCall(OrgLevel.DEPT, d.getId()));  //person on call for whole department, may be null
            returnList.add(d);
        }
        rs.close();
        s.close();
        conn.close();

        return returnList;
    }

    public static List<Office> getOffices(Department d) throws ClassNotFoundException, SQLException {
        List<Office> returnList = new ArrayList();
        Connection conn = getConnection();
        String query = "select id, name from office where department=" + d.getId() + ";";
        Statement s = conn.createStatement();
        ResultSet rs = s.executeQuery(query);
        while (rs.next()) {
            Office o = new Office();
            o.setId(rs.getInt("id"));
            o.setName(rs.getString("name"));
            o.setOnCall(getOnCall(OrgLevel.OFF, o.getId()));  //not supposed to be null, but may be
        }
        rs.close();
        s.close();
        conn.close();

        return returnList;
    }

    public static OnCall getOnCall(OrgLevel orgLevel, int id) throws ClassNotFoundException, SQLException {
        SimpleDateFormat sdf = new SimpleDateFormat("yyyy-MM-dd HH:MM:SS");
        OnCall returnValue = new OnCall();
        Connection conn = getConnection();
        String query = "SELECT id, name, phone, datestart, dateend FROM "
            + " oncall WHERE " + orgLevel.getOrgLevel() + "=" + id + " "
            + "AND (datestart <='" + sdf.format(new Date()) + "' "
            + "AND dateEnd >='" + sdf.format(new Date()) + "');";
        Statement s = conn.createStatement();
        ResultSet rs = s.executeQuery(query);

        while (rs.next()) {
            returnValue.setId(rs.getInt("id"));
            returnValue.setName(rs.getString("name"));
            java.sql.Timestamp jstStart = rs.getTimestamp("datestart");
            returnValue.setStartDateTime(new Date(jstStart.getTime()));
            java.sql.Timestamp jstEnd = rs.getTimestamp("dateEnd");
            returnValue.setEndDateTime(new Date(jstEnd.getTime()));
        }
        rs.close();
        s.close();
        conn.close();

        return returnValue;
    }
}

Office, Organization, and Department tables are simply id, name and (for department and office) id of primary organization/department. OnCall table:

+--------------+----------+------+-----+---------+----------------+
| Field        | Type     | Null | Key | Default | Extra          |
+--------------+----------+------+-----+---------+----------------+
| id           | int(8)   | NO   | PRI | NULL    | auto_increment |
| name         | tinytext | NO   |     | NULL    |                |
| phone        | tinytext | NO   |     | NULL    |                |
| office       | int(8)   | YES  |     | NULL    |                |
| department   | int(8)   | YES  |     | NULL    |                |
| organization | int(8)   | YES  |     | NULL    |                |
| dateStart    | datetime | YES  |     | NULL    |                |
| dateEnd      | datetime | YES  |     | NULL    |                |
+--------------+----------+------+-----+---------+----------------+

Critique, comments, suggestions are very welcome.

Answer 1

• It's good practice to always use prepared statements, as it increases security as well as readability.
• You don't need empty constructors.
• SQL statements get easier to read when you write the SQL keywords in all upper-case.
• Don't use one-character variable names, it makes the code really hard to read. The same also goes for two and three letter variable names. So write organizationName instead of s, statement instead of s, resultSet instead of rs, organization instead of o, connection instead of conn, department instead of d, and so on.
• On the other hand, you do not have to add the variable type to its name. id is just fine, no need for idInt.
• I'm not sure why you can't use the same connection. You could either open it up in the constructor and use it in all methods, or you could construct it somewhere outside this class, and then pass it to each method as a parameter.

Answer 2

I know SQL isn't case-sensitive, but our minds are:

    String query = "SELECT id, name, phone, datestart, dateend FROM "
        + " oncall WHERE " + orgLevel.getOrgLevel() + "=" + id + " "
        + "AND (datestart <='" + sdf.format(new Date()) + "' "
        + "AND dateEnd >='" + sdf.format(new Date()) + "');";

You have both dateend and dateEnd here, referring to the same field - I'd rewrite this query with PascalCase column names (that's how they're actually named in the database anyway, right?).

I don't know what API you're using for querying - I'm not all that familiar with java and mysql data access. But what I do know, is that you're concatenating the parameter values into your query string:

String query = "select id, name from office where department=" + d.getId() + ";";

The above is called in a loop, and every time it hits the server, a new query plan needs to be created because it's a brand new query every time - even if it only differs by the parameter value: the database doesn't even know there's a parameter involved. Compare to this:

String query = "select id, name from office where department=?";

The syntax may vary depending on your driver and provider, but at the end of the day the idea is to always send the same query to the server, so that it can say "hey, I've seen this before, I already know how to do this".

By letting the database deal with the parameters, you're also protecting yourself against SQL injection - which may or may not be a concern, but concatenating parameters into the query string is a bad practice that's always good to lose nonetheless.