JDBC Code Review

[Danish Shaukat]

Hi all,

I am currently working on a discussion forum application.

The method below is used to start a new thread. Multiple tables are being updated (inserts and updates). Some tables are being queried for data required for updates.

Transaction management is in place.

Apart from this I also have a custom exception which is thrown in case of an empty result set.

If any kind of exception is thrown, the transaction is rolled back. I am also rethrowing the exception to the calling program for some further processing.

Can any one go through the code and suggest how to improve it? There is always room for optimization and refactoring so all suggestions are welcomed.

Regards,
Danish

    

package com.maximal.k20a;

import java.sql.*;
import java.util.*;

public class Forum
{
    // Member variables for database connectivity
    private String dbDriver;
    private String dbName;
    private String dbUser;
    private String dbPassword;
   
    public Forum(String driver, String name, String user, String password)
    {
        dbDriver = driver;
        dbName = name;
        dbUser = user;
        dbPassword = password;
    }

   
    public int startNewThread(String userIdent, String forumIdent,
            String subject, String text) throws SQLException,
            ClassNotFoundException, NoRecordFoundException,
            NoRecordUpdatedException
    {
        Connection connection=null;
        int numRecords = 0;

        try
        {
            // Load the driver
            Class.forName(dbDriver);

            // Establish connection
           connection = DriverManager
                    .getConnection(dbName, dbUser, dbPassword);

            // Disable auto commit
            connection.setAutoCommit(false);

            // Create the sql statement
            sql = "select * from thread order by threadid desc limit 1;";

            // Create the PreparedStatement object
            PreparedStatement selectThread = connection.prepareStatement(sql);

            // Execute the statement and get the result set
            ResultSet threadRS = selectThread.executeQuery();

            String threadIdStr = new String();
            int threadIdent = 0;

            if (threadRS.next())
            {
                threadIdStr = threadRS.getString("threadid");
                threadIdStr = threadIdStr.trim();
                System.out.println("Thread Id found: " + threadIdStr);
                threadIdent = Integer.parseInt(threadIdStr);
                threadIdent = threadIdent + 1;
                System.out.println("New thread Id: " + threadIdent);
            }
            else
            {
                // throw NoRecordFoundException
                // this is a custom exception
                throw new NoRecordFoundException("No record found");
            }

            String threadId = new String(Integer.toString(threadIdent));

            threadRS.close();
            selectThread.close();
            threadRS = null;
            selectThread = null;

            // get Unix time stamp value in seconds
            long unixTimestamp = System.currentTimeMillis() / 1000;
            String unixTimestampStr = Long.toString(unixTimestamp);

            // Create the sql statement
            sql = "insert into thread (title, firstpostid, lastpost, forumid, "
                    + "pollid, open, postusername, postuserid, lastposter, "
                    + "dateline, visible) values (?, ?, ?, ?, ?, ?, ?, ?, ?, ?,?);";

            // Create the PreparedStatement object
            PreparedStatement insertThread = connection.prepareStatement(sql);

            // Clear the parameters
            insertThread.clearParameters();

            // Specify the parameters
            insertThread.setString(1, subject);
            insertThread.setString(2, "1");
            insertThread.setString(3, unixTimestampStr);
            insertThread.setString(4, forumIdent);
            insertThread.setString(5, "0");
            insertThread.setString(6, "1");
            insertThread.setString(7, userIdent);
            insertThread.setString(8, userNum);
            insertThread.setString(9, userIdent);
            insertThread.setString(10, unixTimestampStr);
            insertThread.setString(11, "1");

            // Execute the statement
            numRecords = insertThread.executeUpdate();

            System.out.println("Record added to thread table");

            insertThread.close();
            insertThread = null;

            sql = "insert into post (threadid, parentid, username, userid,"
                    + "title, dateline, pagetext, allowsmilie, showsignature, "
                    + "ipaddress, iconid, visible, attach, importthreadid,"
                    + "importpostid, infraction, reportthreadid) values "
                    + "(?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?);";

            // Create the PreparedStatement object
            PreparedStatement insertPost = connection.prepareStatement(sql);

            // Clear the parameters
            insertPost.clearParameters();

            // Specify the parameters
            insertPost.setString(1, threadId);
            insertPost.setString(2, "0");
            insertPost.setString(3, userIdent);
            insertPost.setString(4, userNum);
            insertPost.setString(5, subject);
            insertPost.setString(6, unixTimestampStr);
            insertPost.setString(7, text);
            insertPost.setString(8, "0");
            insertPost.setString(9, "1");
            insertPost.setString(10, "127.0.0.1");
            insertPost.setString(11, "1");
            insertPost.setString(12, "1");
            insertPost.setString(13, "0");
            insertPost.setString(14, "0");
            insertPost.setString(15, "0");
            insertPost.setString(16, "0");
            insertPost.setString(17, "0");

            // Execute the statement
            numRecords = insertPost.executeUpdate();

            System.out.println("Record added to post table");

            insertPost.close();
            insertPost = null;

            // Get Forum Statistics

            // Create the SQL statement
            sql = "select * from forum where forumid=?";

            // Create the PreparedStatement object
            PreparedStatement selectForum = connection.prepareStatement(sql);

            // Clear the parameters
            selectForum.clearParameters();

            // Specify the parameters
            selectForum.setString(1, forumIdent);

            // Execute the query
            ResultSet forumRS = selectForum.executeQuery();

            String threadCountStr = new String();
            String replyCountStr = new String();

            if (forumRS.next())
            {
                replyCountStr = forumRS.getString("replycount");
                replyCountStr = replyCountStr.trim();
                threadCountStr = forumRS.getString("threadcount");
                threadCountStr = threadCountStr.trim();
            }
            else
            {
                // throw NoRecordFoundException
                // this is a custom exception
                throw new NoRecordFoundException("No record found");
            }

            forumRS.close();
            selectForum.close();
            forumRS = null;
            selectForum = null;

            int replyCount = Integer.parseInt(replyCountStr);
            int threadCount = Integer.parseInt(threadCountStr);

            replyCount = replyCount + 1;
            threadCount = threadCount + 1;

            replyCountStr = Integer.toString(replyCount);
            threadCountStr = Integer.toString(threadCount);

            // Update Forum Statistics

            sql = "update forum set replycount=?, lastpost=?, lastposter=?, "
                    + "lastthread=?, lastthreadid=?, threadcount=? where "
                    + "forumid=?";

            // Create the PreparedStatement object
            PreparedStatement updateForum = connection.prepareStatement(sql);

            // Clear the parameters
            updateForum.clearParameters();

            // Specify the parameters
            updateForum.setString(1, replyCountStr);
            updateForum.setString(2, unixTimestampStr);
            updateForum.setString(3, userIdent);
            updateForum.setString(4, subject);
            updateForum.setString(5, threadId);
            updateForum.setString(6, threadCountStr);
            updateForum.setString(7, forumIdent);

            // Execute the SQL statement
            numRecords = updateForum.executeUpdate();

            System.out.println("Forum table updated");

            updateForum.close();
            updateForum = null;

            // Upate user table
            sql = "select * from user where username=?";

            // Create the PreparedStatement object
            PreparedStatement selectUser = connection.prepareStatement(sql);

            // Clear the parameters
            selectUser.clearParameters();

            // Specify the parameters
            selectUser.setString(1, userIdent);

            // Execute the SQL statement
            ResultSet userRS = selectUser.executeQuery();

            String postCountStr = new String();

            if (userRS.next())
            {
                postCountStr = userRS.getString("posts");
                postCountStr = postCountStr.trim();
                int postCount = Integer.parseInt(postCountStr);
                postCount = postCount + 1;
                postCountStr = Integer.toString(postCount);
            }
            else
            {
                // throw NoRecordFoundException
                // this is a custom exception
                throw new NoRecordFoundException("No record found");
            }

            userRS.close();
            selectUser.close();
            userRS = null;
            selectUser = null;

            sql = "update user set lastpost=?, posts=? where username=?";

            // Create the PreparedStatement object
            PreparedStatement updateUser = connection.prepareStatement(sql);

            // Clear the parameters
            updateUser.clearParameters();

            // Specify the parameters
            updateUser.setString(1, unixTimestampStr);
            updateUser.setString(2, postCountStr);
            updateUser.setString(3, userIdent);

            // Execute the SQL statement
            numRecords = updateUser.executeUpdate();

            System.out.println("User table updated");

            updateUser.close();
            updateUser = null;

            // Enable auto commit
            connection.setAutoCommit(true);

            // close the connection
            connection.close();
            connection = null;
        }
        catch (Exception ex)
        {
            //Handle the exception
            numRecords = 0;
            System.out.println("SQLException: " + ex.getMessage());

            if (connection != null)
            {
                System.out.println("Rolling back transaction");
                connection.rollback();
            }

            //Rethrow the exception to the calling program
            throw new SQLException("Transaction failure");
        }

        return numRecords;
    }
}

[Sebastian Janisch]

Your code looks decent, but it's a huge load of boilerplate. Why don't you check out JDBCSupport ? It can minimize the code for you ;-)

[Danish Shaukat]

Sebastian Janisch wrote:Your code looks decent, but it's a huge load of boilerplate. Why don't you check out JDBCSupport ? It can minimize the code for you ;-)

Yes, I agree that the method is really long. Checked out JDBC Support. It is still in beta and was released recently.

[Sebastian Janisch]

Danish Shaukat wrote:
Yes, I agree that the method is really long. Checked out JDBC Support. It is still in beta and was released recently.

It's my project and it's quite young. Check it out and let me know how it's going ;-)

[Wilson Cursino]

Hi Danish,

Looks neat the code, however even for a basic code you should consider the JDBCSupport.

However, in MHO I think you should consider use hibernate, it will turn your life so easy and it isn't that hard.

Cheers and good luck!

[Jeanne Boyarsky]

My comments:
1) I would use a ConnectionPool rather than obtaining the connection via username/password each time.
2) I agree on the boilerplate comment. Many people write their own superclass with the boilerplate or use an ORM framework like Hibernate.
3) startNewThread() is way too long. While the exact length of a method is debatable, most people agree is should fit on a screen or two. Look for snippets you can extract into new methods - like reading from the resultset.
4) I would list out the columns rather an "select *". In fact, I would also specifically list the columns I needed - in this case threadId. Returning columns you don't need as a performance impact.
5) threadRS.close() should be in a finally block.
6) Having to null out variables is an anti-pattern that signifies the code that uses those variables should have been in a separate method.
7) I'd name "selectThread" as "selectTheadStmt". The former sounds like it is of Thread type.
8) "// get Unix time stamp value in seconds " - These comments are also an anti-pattern that the code the comment applies to should be in method named getUnixTimeStampValueInSeconds() and the like.
9) Why do you clear the parameters when the prepared statement has just been created?

[Jeanne Boyarsky]

On the length thing, it isn't just boilerplate. It is that there is so much going on that it is hard for a reader to figure out what the code does. What if someone other than you has to maintain the code? For that matter, what if you have to in a year?

Compare the code in the first post to the following. Your method names would be clearer of course as they could describe what the queries do. They'd also be more real and pass the proper variables around.

    

package com.maximal.k20a;

import java.sql.*;
import java.util.*;

public class Forum
{
    // Member variables for database connectivity
    private String dbDriver;
    private String dbName;
    private String dbUser;
    private String dbPassword;
   
    public Forum(String driver, String name, String user, String password)
    {
        dbDriver = driver;
        dbName = name;
        dbUser = user;
        dbPassword = password;
    }

   
    public int startNewThread(String userIdent, String forumIdent,
            String subject, String text) throws SQLException,
            ClassNotFoundException, NoRecordFoundException,
            NoRecordUpdatedException
    {
        try
        {
            String threadId = findThreadId();
            insertThread();
            insertPost();
            getForumStatistics();
            incrementReplyCount();
            incrementThreadCount();
            updateForumStatistics();
            updateuser();
        }
        catch (Exception ex)
        {
           handleRollBack();
        }

        return numRecords;
    }
}

In addition to fitting on a screen, my version has business meaning - you can tell why each code block is needed. It also provides chunks that are possible to reuse.

[edited to fix typo]

[Danish Shaukat]

Hi Wilson and Jeanne,

Thanks for going through the lengthy code. Your comments are valuable. I will be changing the code as per guidelines.

Regards,
Danish

[Wilson Cursino]

Danish you are welcome. Let me know if you need any help with the conversion.