5
\$\begingroup\$

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();
    }
}
\$\endgroup\$
  • \$\begingroup\$ Are you on Java 8? \$\endgroup\$ – h.j.k. Oct 14 '15 at 23:45
  • 1
    \$\begingroup\$ @h.j.k Yes I am \$\endgroup\$ – Dan Oct 14 '15 at 23:48
6
\$\begingroup\$

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());
    
\$\endgroup\$
  • \$\begingroup\$ You shouldn't just dump code as a review! Please explain what makes your answer better as a comparison to the OP's code! \$\endgroup\$ – IEatBagels Oct 15 '15 at 20:11
  • \$\begingroup\$ @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 :) \$\endgroup\$ – fwolfe Oct 16 '15 at 16:11
  • \$\begingroup\$ Yeah that's really good! \$\endgroup\$ – IEatBagels Oct 16 '15 at 16:34
  • \$\begingroup\$ @TopinFrassi Thanks for all the advice! Any additional advice you might have using Java 8? \$\endgroup\$ – Dan Oct 16 '15 at 18:00
  • \$\begingroup\$ I didn't get to use Java8 yet :p But I can spot a good answer when I see one :p \$\endgroup\$ – IEatBagels Oct 16 '15 at 18:00
1
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ 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? \$\endgroup\$ – Dan Oct 15 '15 at 15:27
  • \$\begingroup\$ @Dan yes you should, since the PreparedStatement object you have created here does not need to be referenced outside of the method. \$\endgroup\$ – h.j.k. Oct 15 '15 at 15: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.