2
\$\begingroup\$

I have come up with the following method to post a transaction to a database, in the context of an e-commerce Java web application. The transaction consists in submitting a new order alongside its details:

public long insertOrder(Customer customer, Hashtable<String, CartItem> shoppingCart)
{
    long returnValue = 0L;
    Connection connection = null;
    Statement statement = null;
    try {
        connection = MySqlConnection.getConnection();
        connection.setAutoCommit(false);
        statement = connection.createStatement();
        long orderId = System.currentTimeMillis();
        OrderPeer.insertOrder(statement, orderId, customer);
        OrderDetailsPeer.insertOrderDetails(statement, orderId, shoppingCart);
        connection.commit();
        returnValue = orderId;
    } catch (SQLException e) {
        try { connection.rollback(); } catch (Exception ex) { }
        System.out.println("Could not insert order: " + e.getMessage());
    } finally {
        try { statement.close(); } catch (Exception e) { }
        try { connection.close(); } catch (Exception e) { }
    }
    return returnValue;
}

I would like to know if I am handling the exception, rolling back, and closing the statement/connection in the best possible manner.

\$\endgroup\$
  • \$\begingroup\$ Hi, I have rolled back your update, please see What you may and may not do once you've received answers \$\endgroup\$ – Kaz Jan 18 '16 at 18:27
  • \$\begingroup\$ @Zak Thanks for your accurate correction. I have read the referred resource and added my own answer as a consequence. Please, let me know if there's anything that could use further improvement. \$\endgroup\$ – Enrique Jan 18 '16 at 18:48
  • \$\begingroup\$ Follow-up question here: codereview.stackexchange.com/q/117176/67620 \$\endgroup\$ – Enrique Jan 18 '16 at 19:13
  • \$\begingroup\$ @Zak I have posted a follow-up question, as per your suggestion. Hopefully, I'm doing better now. :) \$\endgroup\$ – Enrique Jan 18 '16 at 19:15
  • \$\begingroup\$ Just make sure to add a link here pointing to your follow-up, and a link in your follow-up pointing here. \$\endgroup\$ – Kaz Jan 18 '16 at 19:48
2
\$\begingroup\$

Using the "try-with-resources" statement should clean up your code significantly. There are two advantages:

  1. It allows you to declare your resources (in this case Connection and Statement objects) in exactly the scope they're going to be used. This means you don't have to first initialize them to null, which is ugly.
  2. It will ensure your resources get closed no matter what.

Also, you should probably print the stack trace (to aid you and others in debugging), and you should print your error messages to stderr instead of stdout.

public long insertOrder(Customer customer, Hashtable<String, CartItem> shoppingCart)
{
    try (Connection connection = MySqlConnection.getConnection();
        Statement statement = connection.createStatement();) {
        connection.setAutoCommit(false);
        long orderId = System.currentTimeMillis();
        OrderPeer.insertOrder(statement, orderId, customer);
        OrderDetailsPeer.insertOrderDetails(statement, orderId, shoppingCart);
        connection.commit();
        return orderId;
    } catch (SQLException e) {
        try {
           connection.rollback();
        } catch (Exception ex) {
            System.err.println("Could not roll back connection: " + ex.getMessage())
            ex.printStackTrace();

        }
        System.err.println("Could not insert order: " + e.getMessage());
        e.printStackTrace()l
        return 0;
    }
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Welcome to Code Review! Would you mind expanding a bit on your advice? \$\endgroup\$ – Mast Jan 2 '16 at 10:27
  • \$\begingroup\$ There are some flaws in your code. For instance, the connection.rollback() is outside the scope of the connection variable, which only exists inside the body of the try construct. \$\endgroup\$ – Enrique Jan 14 '16 at 14:38
  • \$\begingroup\$ I have used your suggestion to create a better version of my code and have added it as an update to my original question. It'd be great if you could take a look at it. \$\endgroup\$ – Enrique Jan 15 '16 at 0:37
  • \$\begingroup\$ Sorry my bad, I didn't realize that connection.rollback() had to be explicitly called before connection.close() when doing a transaction. My experience with JDBC is limited. It looks like the changes you made above solve the problem with my code. It's unfortunate that two try ... catch ... blocks have to be used, but I don't see a cleaner solution. \$\endgroup\$ – gardenhead Jan 15 '16 at 1:04
  • \$\begingroup\$ @gardenhead I have placed a follow-up question with my improved version of the code. I thought you might want to take a look at it. You'll find its link in the comments section of this question. \$\endgroup\$ – Enrique Jan 18 '16 at 19:19
0
\$\begingroup\$

Instead of returning long, with zero representative of either a failure to insert or a failed rollback, suggest returning objects instead. Objects could be created for narrower range of order id's, objects which had instance variable area to store specific success or failure details or provide useful predicate methods: isInsertOK, isRollback, etc.

Would be safer not to use long without any range. Risk may be unlikely now as method is small but suppose some error or corruption occurrs resulting in an invalid order id but valid long. These objects would consolidate repeated procedural code handling return codes, order id's, failures.

\$\endgroup\$

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.