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();
 }
}

• h.j.k.: Are you on Java 8?
• Dan: @h.j.k Yes I am

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());
   

• IEatBagels: You shouldn't just dump code as a review! Please explain what makes your answer better as a comparison to the OP's code!
• fwolfe: @TopinFrassi I had some explanations in the comments, but now I've moved those out to make it easier to read and added explanations for the other parts of the code that I changed. Hope my edit helps :)
• IEatBagels: Yeah that's really good!
• Dan: @TopinFrassi Thanks for all the advice! Any additional advice you might have using Java 8?
• IEatBagels: I didn't get to use Java8 yet :p But I can spot a good answer when I see one :p

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.

• Dan: Yes currently it is a class variable, I have multiple database methods and I use it in each one. Should I scope it down to the method for every method in my class?
• h.j.k.: @Dan yes you should, since the PreparedStatement object you have created here does not need to be referenced outside of the method.