1
\$\begingroup\$

Some days ago, I put up a question here for review about the best way to handle JDBC transactions in the context of an e-commerce Java web application.

After a kind suggestion from user @gardenhead, I decided to use the try-with-resources construct to automatically close my resources, namely the statement and the connection objects.

I also found this SO answer, which told me how to properly use try-with-resources for this kind of operation.

This is my improved code:

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

The rollback is triggered when either there is a problem with the transaction itself, or when there is a problem with the automatic closing of the statement. The later due to the fact that it would be indicative of a database access error that left the transaction in an unknown state.

If there is anything else that could be improved, please let me know.

\$\endgroup\$
  • \$\begingroup\$ Do you need to worry about high volumes where you'll be inserting orders with the same System.currentTimeMillis() (either multi-threaded writes, or just two instances of your application)? \$\endgroup\$ – h.j.k. Jan 19 '16 at 3:50
  • \$\begingroup\$ I have rolled back the last edit. Please see What to do when someone answers. \$\endgroup\$ – h.j.k. Jan 19 '16 at 15:28
1
\$\begingroup\$

Maps

(copying my answer from another question):

A HashMap or ConcurrentHashMap (if you require the thread-safety) is preferred over the legacy Hashtable, as mentioned in the Javadoc:

As of the Java 2 platform v1.2, this class was retrofitted to implement the Map interface, making it a member of the Java Collections Framework. Unlike the new collection implementations, Hashtable is synchronized. If a thread-safe implementation is not needed, it is recommended to use HashMap in place of Hashtable. If a thread-safe highly-concurrent implementation is desired, then it is recommended to use ConcurrentHashMap in place of Hashtable.

Typo?

OrderDetailsPeer.insertOrderDetails(statement, orderId, customer);

Compared to your first version, it seems that shoppingCart should be inserted here instead of customer, unless you have changed your APIs. If so, then shoppingCart is unused in this method...

\$\endgroup\$
  • \$\begingroup\$ You are right. That was a typo. shoppingCart was what I should have written. \$\endgroup\$ – Enrique Jan 19 '16 at 20:00

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.