Now let’s take a stab at analyzing the synergy between the recently discovered ILedgeDAO and IStashDAO implementations i.e. LedgerDAO & StashDAO.
As it stands, LedgerDAO looks something like this:
@Override public void record(LedgerEntry theEntry) throws SQLException { try (Connection conn = DriverManager.getConnection( dbConnectionUrl, dbUserName, dbPassword)) { conn.setAutoCommit(false); try (PreparedStatement stmt = conn.prepareStatement( "insert into transaction_log (amount, description, " + "txntime, txntype) " + "values (?, ?, ?, ?)")) { stmt.setDouble(1, theEntry.getAmount()); stmt.setString(2, theEntry.getDescription()); stmt.setTimestamp(3, new Timestamp(theEntry.getDate().getTime())); stmt.setString(4, theEntry.getCreditOrDebit().name()); int rowsUpdated = stmt.executeUpdate(); if (rowsUpdated != 1) { throw new SQLException("Should have inserted 1 " + "transaction, rows inserted were " + rowsUpdated); } conn.commit(); } catch (SQLException e) { conn.rollback(); throw e; } } }
For StashDAO, let’s just look at one of the methods
@Override public void saveBalance( @NotNull final String stashId, final double balance) throws SQLException { try (Connection conn = DriverManager.getConnection( dbConnectionUrl, dbUserName, dbPassword)) { conn.setAutoCommit(false); try (PreparedStatement stmt = conn.prepareStatement( "update mystash set balance = ?")) { stmt.setDouble(1, balance); int rowsUpdated = stmt.executeUpdate(); if (rowsUpdated != 1) { throw new SQLException("Unexpected error. " + "One row should have been updated, but was " + rowsUpdated); } conn.commit(); } catch (SQLException e) { conn.rollback(); throw e; } } }
What strikes you as similar? Well pretty much a no brainer I think? There is a chunk of code in each which creates a Connection & a Statement and does something with it in both. What that something is, does vary. You could evolve this code by just looking at one of these DAO classes & question whether it is really doing just one thing i.e. has only one and only one reason to change (SRP) or you could realize the duplication just stated i.e. both these do something within a common duplicate code block of creating a Connection & Statement. I would wager that either ways you would evolve the code in the same direction. Most likely if you are in a team, you would also be focusing on just one thing, ;-). So let’s take that route.
Let’s pick up LedgerDAO. Given its current state, what would be the possible reasons that would cause this class to change? At these levels, it may not be very self-evident, after all it is just inserting a record into the table. So here is a trick. Make a list of things it is doing in English. Here goes:
- Create a JDBC connection given the connection information
- Begin a transaction by calling setAutoCommit(false)
- Construct an SQL for inserting a record into the “Ledger” table
- Create a JDBC statement object from the SQL & the JDBC Connection
- Plug in the LedgerEntry data into the JDBC statement
- Execute the JDBC statement
- Commit or roll back the transaction as the case may be
Yes, listed in quite detail I must say. Could have rather just pasted the code instead. no? It helps to lay things down in plain English & bullets so that you can question each bullet, until this style of thinking comes naturally.
Now there are two ways you could go about this. Let’s take an easy way first. You already have the SRP for the method at a high level i.e. from its name – record(LedgerEntry). Let’s attempt to highlight in similar colors the bullets which closely relate to each other, trying to group them, closely related bullets in the same group. This would most likely come very naturally to you. Here is my attempt:
- Create a JDBC connection given the connection information
- Begin a transaction by calling setAutoCommit(false)
- Construct an SQL for inserting a record into the “Ledger” table
- Create a JDBC statement object from the SQL & the JDBC Connection
- Plug in the LedgerEntry data into the JDBC statement
- Execute the JDBC statement
- Commit or roll back the transaction as the case may be
That is a colorful set of bullets! What’s the color of the bullets which directly talk of a LedgerEntry? Yes you got it! 2 out of 7, the remaining is boilerplate code that will be repeated again and again and again across all the DAO(s) you write! ~70% code duplication.
Surely all this boilerplate code should be handed off to one or more guys and each DAO shouldn’t have to worry about this. Let’s now revisit the code and do the lighting in the code itself and see how it looks:
@Override public void record(LedgerEntry theEntry) throws SQLException { try (Connection conn = DriverManager.getConnection( dbConnectionUrl, dbUserName, dbPassword)) { conn.setAutoCommit(false); try (PreparedStatement stmt = conn.prepareStatement( "insert into transaction_log (amount, description, " + "txntime, txntype) " + "values (?, ?, ?, ?)")) { stmt.setDouble(1, theEntry.getAmount()); stmt.setString(2, theEntry.getDescription()); stmt.setTimestamp(3, new Timestamp(theEntry.getDate().getTime())); stmt.setString(4, theEntry.getCreditOrDebit().name()); int rowsUpdated = stmt.executeUpdate(); if (rowsUpdated != 1) { throw new SQLException("Should have inserted 1 " + "transaction, rows inserted were " + rowsUpdated); } conn.commit(); } catch (SQLException e) { conn.rollback(); throw e; } } }
Let’s lay it out again, one more time, and this time, let’s blur out the orange code which directly relates to a LedgerEntry, with a simple “…”
@Override public void record(LedgerEntry theEntry) throws SQLException { try (Connection conn = DriverManager.getConnection( dbConnectionUrl, dbUserName, dbPassword)) { conn.setAutoCommit(false); try (PreparedStatement stmt = conn.prepareStatement(...)) { ... int rowsUpdated = stmt.executeUpdate(); if (rowsUpdated != 1) { throw new SQLException("Should have inserted 1 " + "transaction, rows inserted were " + rowsUpdated); } conn.commit(); } catch (SQLException e) { conn.rollback(); throw e; } // This is pink as it closes the statement ;-) } // This is blue as it closes the connection ;-) }
If only we could pull all these colored lines of code into a common method which all DAO(s) could then use to insert/update data.
Well what are you waiting for? How about this:
@Override public void record(LedgerEntry theEntry) throws SQLException { String insertLedgerEntrySQL = "insert into ledger " + "(amount, description, txntime, txntype) " + "values (?, ?, ?, ?)"; executeSql( insertLedgerEntrySQL, theEntry.getAmount(), theEntry.getDescription(), theEntry.getDate().getTime(), theEntry.getCreditOrDebit().name()); } /** * Client code of this method knows the SQL statement * and hence knows right order the parameters need to be in. */ private void executeSql( String sql, Object... parameters) throws SQLException { try (Connection conn = DriverManager.getConnection( dbConnectionUrl, dbUserName, dbPassword)) { conn.setAutoCommit(false); try (PreparedStatement stmt = conn.prepareStatement(sql)) { for (int index = 1; index <= parameters.length; index++) { stmt.setObject(index, parameters[index]); } int rowsUpdated = stmt.executeUpdate(); if (rowsUpdated != 1) { throw new SQLException("Should have inserted 1 " + "record, rows inserted were " + rowsUpdated); } conn.commit(); } catch (SQLException e) { conn.rollback(); throw e; } // This is pink as it closes the statement ;-) } // This is blue as it closes the connection ;-) }
Let’s not stop here. Really what we want is to have a method or a set of methods for each color. So a blue method, a green method and a pink method all working seamlessly together to achieve the common goal of record(LedgerEntry). We can easily pull out the pink, as it is a good chunk of code sitting together. So:
private void executeSql( String sql, Object... parameters) throws SQLException { try (Connection conn = DriverManager.getConnection( dbConnectionUrl, dbUserName, dbPassword)) { conn.setAutoCommit(false); try { executeSql(sql, conn, parameters); conn.commit(); } catch (SQLException e) { conn.rollback(); throw e; } } } private void executeSql( String sql, Connection conn, Object[] parameters) throws SQLException { try (PreparedStatement stmt = conn.prepareStatement(sql)) { for (int index = 1; index <= parameters.length; index++) { stmt.setObject(index, parameters[index]); } int rowsUpdated = stmt.executeUpdate(); if (rowsUpdated != 1) { throw new SQLException("Should have inserted 1 " + "record, rows inserted were " + rowsUpdated); } } }
Let’s eliminate the detail of creating a connection by pulling it out into its own method, albeit just a one line. So:
private void executeSql( String sql, Object... parameters) throws SQLException { try (Connection conn = getConnection()) { conn.setAutoCommit(false); try { executeSql(sql, conn, parameters); conn.commit(); } catch (SQLException e) { conn.rollback(); throw e; } } } private Connection getConnection() throws SQLException { return DriverManager.getConnection( dbConnectionUrl, dbUserName, dbPassword); }
“executeSQL()” is really all green now as it delegates all the nitty-gritty out and focuses on executing the SQL in the context of a transaction. So here is the full listing:
@Override public void record(LedgerEntry theEntry) throws SQLException { String insertLedgerEntrySQL = "insert into ledger " + "(amount, description, txntime, txntype) " + "values (?, ?, ?, ?)"; executeSql( insertLedgerEntrySQL, theEntry.getAmount(), theEntry.getDescription(), theEntry.getDate().getTime(), theEntry.getCreditOrDebit().name()); } /** * Client code of this method knows the SQL statement * and hence knows right order the parameters need to be in. */ private void executeSql( String sql, Object... parameters) throws SQLException { try (Connection conn = getConnection()) { conn.setAutoCommit(false); try { executeSql(sql, conn, parameters); conn.commit(); } catch (SQLException e) { conn.rollback(); throw e; } } } private Connection getConnection() throws SQLException { return DriverManager.getConnection( dbConnectionUrl, dbUserName, dbPassword); } private void executeSql( String sql, Connection conn, Object[] parameters) throws SQLException { try (PreparedStatement stmt = conn.prepareStatement(sql)) { for (int index = 1; index <= parameters.length; index++) { stmt.setObject(index, parameters[index]); } int rowsUpdated = stmt.executeUpdate(); if (rowsUpdated != 1) { throw new SQLException("Should have inserted 1 " + "record, rows inserted were " + rowsUpdated); } } }
And clearly these methods, besides the one in orange, needs a place which can be leveraged by all DAO(s).
Where? The easiest way to solve this is to realize that the top method, the green one, is the goal and the chain of methods that it lands up calling is the implementation. So create a class equivalent to this method name and move the code into it. So executeSql is the method name, let’s call the class SQLExecutor for now. So:
public class LedgerJDBCDAO implements ILedgerDAO { private SQLExecutor sqlExecutor; public LedgerJDBCDAO(SQLExecutor sqlExecutor) { this.sqlExecutor = sqlExecutor; } @Override public void record(LedgerEntry theEntry) throws SQLException { String insertLedgerEntrySQL = "insert into ledger " + "(amount, description, txntime, txntype) " + "values (?, ?, ?, ?)"; sqlExecutor.executeSql( insertLedgerEntrySQL, theEntry.getAmount(), theEntry.getDescription(), theEntry.getDate().getTime(), theEntry.getCreditOrDebit().name()); } }
Notice how the class has reduced in lines of code. Also notice how the LedgerJDBCDAO now takes the SQLExecutor, rather than the low level details of connection parameters. And most importantly, how the LedgerJDBCDAO now focuses on the SQL query it knows and the data that is needed to populate it. Delegates everything else out.
Guess what the StashDAO.saveBalance() looks like right now:
@Override public void saveBalance( @NotNull final String stashId, final double balance) throws SQLException { String updateBalanceSql = "update mystash set balance = ?"; sqlExecutor.executeSql( updateBalanceSql, balance); }
Lets see the fetchBalance() method. Smell a problem? Yes? Unlike the earlier methods, this is a select rather than an insert, update, delete. This returns data. So while the input parameters to SQLExecutor.executeSQL() are fine, clearly a void return won’t work. This is a different semantic. No worries, we need another method in SQLExecutor that caters to select(s).
@Override public double fetchBalance( @NotNull final String stashId) throws SQLException { String fetchBalanceSql = "select balance from mystash"; try (ResultSet resultSet = sqlExecutor.executeQuery(fetchBalanceSql)) { return resultSet.getDouble("balance"); } }
But wait? Are we missing something? This may be subtle to some new starters, but the problem should hopefully strike the more experienced programmers who have dealt with firing database queries for a long time now.
Any clue?
Take a look at our executeSQL() method listed way above. Repeated below for your easy perusal:
private void executeSql( String sql, Connection conn, Object[] parameters) throws SQLException { try (PreparedStatement stmt = conn.prepareStatement(sql)) { for (int index = 1; index <= parameters.length; index++) { stmt.setObject(index, parameters[index]); } int rowsUpdated = stmt.executeUpdate(); if (rowsUpdated != 1) { throw new SQLException("Should have inserted 1 " + "record, rows inserted were " + rowsUpdated); } } }
How about now? The PreparedStatement is closed using a try-with-resources. As you cater to the new requirement of a select, you “may” have added the following method to SQLExecutor:
private ResultSet executeQuery( String querySql, Connection conn, Object[] parameters) throws SQLException { try (PreparedStatement stmt = conn.prepareStatement(querySql)) { for (int index = 1; index <= parameters.length; index++) { stmt.setObject(index, parameters[index]); } return stmt.executeQuery(); } }
Let’s read the JavaDoc of Statement.close(). The key parts repeated below, again for your easy perusal:
* Note: When a Statement object is * closed, its current ResultSet object, if one exists, is * also closed.
So by the time you start fetching the balance in StashDAO, the ResultSet has already been closed! So unfortunately, you are not going to get squat! You are going to land up with a SQLException stating the same. So quite a different semantic with this query method you need. Lesson learnt: Don’t blindly copy paste! Always respect and read JavaDoc of API you consume.
So what now? Clearly you can’t close the PreparedStatement and as a matter of fact, the database Connection(!) until you have read the balance from the ResultSet.
Boy! Just when you thought this post was drawing to a close, this springs up! Phew! Proceed? Take a break?
There is actually an easy solution to this, but there is indeed a larger conversation about transactions & the service layer to be had.
The easy solution, code-wise, is simple. But, am trying to think of the best way to put it here. Don’t really want to burden this conversation by using the term “Lambda(s)”, since from an interface/modelling point of view, something equivalent to “Lambda(s)” were always possible. Think of the very old Swing ActionListener interface, the Observer pattern. Even the term “Call-back” if you will. Any of these strike an idea?
Thinking about the “easy” solution conjures up stories of “spies” and “agents”, at least in my mind! The more I think about it, I feel it is worthy of its own post. Infact with all this hint dropping, you might even come up with your own spy story!
So decision made, sorry!…the spy story I come up with….next post.
Ranjan George 