Inserting a Java object into a MySQL database

Question

I'm writing a method to insert objects into a MySQL database and while everything runs fine I feel my code is bulky and inefficient. What steps would you take to make this code more pleasant to look at and more efficient?

public void addAuthorization(Authorization authorization) {
    try {
        // Builds the query, lastHeader is used to prevent a trailing comma
        query = "INSERT into authorizations (";
        for(int i = 0; i < authorizationsHeaders.size() - 1; i++) {
            query += authorizationsHeaders.get(i) + ", ";
            questionMarks += "?, ";
        }
        String lastHeader = authorizationsHeaders.get(authorizationsHeaders.size() - 1);
        query += lastHeader + ") values(" + questionMarks + "?)";

        openConnection();
        ps = con.prepareStatement(query);
        ps.setString(1,  authorization.getCompany());
        ps.setString(2,  authorization.getPromoType());
        ps.setString(3,  authorization.getPromoDescription());
        ps.setString(4,  authorization.getStartDate());
        ps.setString(5,  authorization.getEndDate());
        ps.setString(6,  authorization.getVlMarketingNum());
        ps.setString(7,  authorization.getMarketingNum());
        ps.setString(8,  authorization.getStatus());
        ps.setDouble(9,  authorization.getForecast());
        ps.setDouble(10, authorization.getActual());

        ps.executeUpdate();
    } catch (SQLException e) {
        e.printStackTrace();
    } finally {
        closeConnection();
    }
}

Answer 1

If you don't have Java 8, here are a few things you can do.

1. Use a string builder instead of concatenating string together. Each time you concatenate to a string, a new string is created. Using a String builder is a more efficient way of doing the same thing.

    StringBuilder sb = new StringBuilder("INSERT into authorizations (");
    sb.append("another string");
    sb.append("another string");
    String query = sb.toString();
   

2. You can use String.join to generate a comma separated string of headers

       String headers = String.join(",", authorizationHeaders);
       sb.append(headers);
   

3. I'm not sure the best way to generate the list of question marks, but this is how I normally do it.

       sb.append( ") values(");
       boolean first = true;
       for(String header: authorizationHeaders) {
           if(first){
               sb.append("?");
               first= false;
           }else{
               sb.append("?,");
           }
   
       }
      sb.append(")");
   

4. When setting your parameters, using i++ instead of hardcoded numbers makes code maintenance easier. If you end up needing to add one more setString(), you can add the one line of code without having adjust the rest of the indexes

       int i=1;
       ps.setString(i++,  authorization.getCompany());
       ps.setString(i++,  authorization.getPromoType());
       ps.setString(i++,  authorization.getPromoDescription());
       ps.setString(i++,  authorization.getStartDate());
       ps.setString(i++,  authorization.getEndDate());
       ps.setString(i++,  authorization.getVlMarketingNum());
       ps.setString(i++,  authorization.getMarketingNum());
       ps.setString(i++,  authorization.getStatus());
       ps.setDouble(i++,  authorization.getForecast());
       ps.setDouble(i++, authorization.getActual());
   

Answer 2

Using Java 8, there are arguably shorter ways of creating your INSERT parameterized statement by using Collectors.joining() on streams:

// assuming authorizationsHeaders is a Collections object
String columns = authorizationsHeaders.stream().collect(Collectors.joining(", ",
                    "INSERT INTO authorizations (", ")"));
String placeHolders = Collections.nCopies(authorizationsHeaders.size(), "?").stream()
                        .collect(Collectors.joining(", ", " VALUES (", ")"));
String statement = columns + placeHolders;

edit: I just realized there is no type declaration for ps... does that mean it is actually a class variable? If so, I will suggest scoping it down to within the method (or within the try-catch block, even), as I don't think there is a need for each PreparedStatement object to be referenced outside of the method it is created.

edit #2: Looking at these two lines:

openConnection();
ps = con.prepareStatement(query);

openConnection() is what I call a 'magic' method, as it is not clear from this usage how it is ensuring the Connection object con is usable on the next line. Assuming that you are already using the recommended practice of pooling your JDBC connections using a well-tested pooling manager, it will be better to make this less magical:

Connection connection = getConnection(); // establishes a working connection
PreparedStatement ps = connection.prepareStatement(statement);
// ...

Same advice for closeConnection(). Depending on the pooling manager's implementation, some handle the calling of the Connection.close() method as an indication to recycle the connection within its pool. Even if you are not pooling JDBC connections at this point in time, you shouldn't require a 'magic' method to handle the termination of the connection. The point is, either ways you should be calling connection.close() directly to also make it explicit when the connection is no longer required.