-
Notifications
You must be signed in to change notification settings - Fork 71
Description
Currently, TestTransaction implements TransactionBase, but still has a commit() and close() method with the same signature as the Transaction interface, which itself extends AutoCloseable. It should implement Transaction instead.
TestTransaction also has a done() method that does:
try {
commit();
} finally {
close();
}Once TestTransaction is AutoCloseable, inheriting that from Transaction, the calls to done() can, and should, be converted to calls to commit() at the end of a try-with-resources block, and the done() method should be removed. This will make the lifecycle management of test transactions in the test code more clear, rather than relying on the implied close() method inside the done(). It will also make the order of commit() calls more clear when writing/maintaining test code. With commit() hidden inside the done() method, it's not immediately obvious when they are called.
This proposal would cause code written like:
TestTransaction tx = new TextTransaction();
// ... stuff here
tx.done(); // implied commit() and close()to be written more explicitly as:
try (TestTransaction tx = new TextTransaction()) {
// ... stuff here
tx.commit(); // explicit commit()
} // auto closedThis example doesn't look that much better, but it's a big improvement when there are multiple transactions being created, committed, and closed in a test.