2
\$\begingroup\$

I am trying to insert into a database using JDBC and each thread will be inserting into the database. I need to insert around 30-35 columns. I wrote a stored procedure that will UPSERT into those columns.

The problem I am facing is that in the run() method, I have around 30 columns written over there for insertion. Is there any way I can simplify my run method so that it doesn't looks so messy which is looking right now for me? And I have a few more columns as well. So if I keep on adding new columns there, it will be looking so messy at one point in my run method.

Is there any way to clean this, keeping in mind the thread safety issues?

class Task implements Runnable {

    private Connection dbConnection = null;
    private CallableStatement callableStatement = null;

    public Task() {

    }

    @Override
    public void run() {

    dbConnection = getDbConnection();
    callableStatement = dbConnection.prepareCall(Constants.UPSERT_SQL);

    callableStatement.setString(1, String.valueOf(userId));
    callableStatement.setString(2, Constants.getaAccount(userId));
    callableStatement.setString(3, Constants.getaAdvertising(userId));
    callableStatement.setString(4, Constants.getaAvgSellingPriceMainCats(userId));
    callableStatement.setString(5, Constants.getaCatAndKeywordRules(userId));
    callableStatement.setString(6, Constants.getaClvBehavior(userId));
    callableStatement.setString(7, Constants.getaClvChurn(userId));
    callableStatement.setString(8, Constants.getaClvInfo(userId));
    callableStatement.setString(9, Constants.getaClvSegmentation(userId));
    callableStatement.setString(10, Constants.getaCsaCategoriesPurchased(userId));
    callableStatement.setString(11, Constants.getaCustomerService(userId));
    callableStatement.setString(12, Constants.getaDemographic(userId));
    callableStatement.setString(13, Constants.getaFinancial(userId));
    callableStatement.setString(14, Constants.getaGeolocation(userId));
    callableStatement.setString(15, Constants.getaInterests(userId));
    callableStatement.setString(16, Constants.getaLastContributorsPurchased(userId));
    callableStatement.setString(17, Constants.getaLastItemsLost(userId));
    callableStatement.setString(18, Constants.getaLastItemsPurchased(userId));
    callableStatement.setString(19, Constants.getaLastProductsPurchased(userId));
    callableStatement.setString(20, Constants.getaLastSellersPurchasedFrom(userId));
    callableStatement.setString(21, Constants.getaMainCategories(userId));
    callableStatement.setString(22, Constants.getaMessaging(userId));
    callableStatement.setString(23, Constants.getaPositiveSellers(userId));
    callableStatement.setString(24, Constants.getaPromo(userId));
    callableStatement.setString(25, Constants.getaScores(userId));
    callableStatement.setString(26, Constants.getaSegmentation(userId));
    callableStatement.setString(27, Constants.getaSellers(userId));
    callableStatement.setString(28, Constants.getaSrpBuyerUpiCount(userId));
    }
  }



    private Connection getDBConnection() {

    Connection dbConnection = null;

          Class.forName(Constants.DRIVER_NAME);
      dbConnection = DriverManager.getConnection(url,user,password);

      return dbConnection;
    }
\$\endgroup\$
7
  • 3
    \$\begingroup\$ I have the felling that your Constants are not constant. \$\endgroup\$ Feb 9, 2013 at 7:55
  • \$\begingroup\$ Yeah, they are not constants. Before posting here I have renamed the name just to shorten it. \$\endgroup\$
    – user21973
    Feb 9, 2013 at 8:09
  • \$\begingroup\$ As callableStatement is an instance field it is not shared and there is no need to synchronize it. \$\endgroup\$ Feb 9, 2013 at 8:27
  • \$\begingroup\$ Thanks mnhg for the suggestion. In general how do we decide when we want to synchronize the method? If you cane explain me then I can understand more. Thanks for the help. \$\endgroup\$
    – user21973
    Feb 9, 2013 at 8:59
  • \$\begingroup\$ Can you provide a bit more detail on what the "constant" methods are doing? Are they retrieving a simple value, or calculating some things? Given the only argument is userId it looks like it might be a simple lookup. Is it from a database, or something hard-coded, or previously read in from the database? If there is an explicit table mapping userId to various values you could simplify things by iterating the table to create the callableStatement. Any new additions to the table would automatically be picked up then when creating the statement. \$\endgroup\$
    – Sean
    Feb 9, 2013 at 13:21

2 Answers 2

4
\$\begingroup\$

Extract a method.

class Task implements Runnable {

    ... 
    private completeStatement (CallableStatement stmt, Strung userId)
    {
        callableStatement.setString(1, String.valueOf(userId));
        ...
    }  

    @Override
    public void run() {
        dbConnection = getDbConnection();
        callableStatement = dbConnection.prepareCall(Constants.UPSERT_SQL);
        completeStatement(callableStatement,userId) 

    }
}

private Connection getDBConnection() {
    ...
}

But find a more suitable name for what your statement is doing.

\$\endgroup\$
5
  • \$\begingroup\$ I was not able to understand what do you mean? Can provide some example so that I can understand better? \$\endgroup\$
    – user21973
    Feb 9, 2013 at 8:08
  • \$\begingroup\$ I update the code. \$\endgroup\$ Feb 9, 2013 at 8:15
  • \$\begingroup\$ Thanks mnhg for the suggestion. Now it is more clear. One more doubt I have is, in this case we won't be making completeStatement method synchronized? \$\endgroup\$
    – user21973
    Feb 9, 2013 at 8:19
  • \$\begingroup\$ There are many discussion on stackoverflow regarding this topic: start here \$\endgroup\$ Feb 9, 2013 at 9:10
  • \$\begingroup\$ +1 because the first step is to isolate the code in its own method. I think more may be possible, depending what those Constants.xxx() methods are doing. \$\endgroup\$
    – Sean
    Feb 9, 2013 at 13:26
2
\$\begingroup\$

Some minor notes:

  1. dbConnection and callableStatement might be local variables.

  2. If more than one thread calls the run method at the same time the shared dbConnection and callableStatement references might lead to resource leaks/race conditions.

  3. I'd consider using setString(String parameterName, String x) which uses readable parameter names instead of integer indexes.

\$\endgroup\$
1
  • 1
    \$\begingroup\$ (10k! Whoah! Congrats!) \$\endgroup\$ Feb 18, 2013 at 14:43

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.