Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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

Problem I am facing is if you look at my 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 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.

Any way to make this thing to look more cleaner keeping in mind of thread safety issues?

Below is my code-

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;
    }

Updated Code:-

This my main thread code from which I am creating threads-

//create thread pool with given size 
ExecutorService service = Executors.newFixedThreadPool(noOfThreads);

try {
    // queue some tasks
    for (int i = 0; i < noOfTasks * noOfThreads; i++) {
        service.submit(new Task());
    }
    service.shutdown();
    service.awaitTermination(Long.MAX_VALUE, TimeUnit.SECONDS);

    while (!service.isTerminated()) {

    }
} catch (Exception e) {

}
share|improve this question

migrated from stackoverflow.com Feb 12 '13 at 13:09

This question came from our site for professional and enthusiast programmers.

    
You can try my suggestion from this answer –  Luiggi Mendoza Feb 9 '13 at 7:20

3 Answers 3

You can use the following utility method:

public static void setStrings (CallableStatement stmt, Object ... values)
{
    for (int count = values.length, i = 0; i < count; i++)
        stmt.setString (i + 1, String.valueOf (values [i]));
}

in your run() like this:

setStrings (
    callableStatement,
    userId,
    Constants.getaAccount (userId),
    Constants.getaAdvertising (userId),
    ...
    Constants.getaSrpBuyerUpiCount (userId));

This is shorter and much more readable for me.

share|improve this answer
    
Thanks Mikhail for the suggestion. So that means I should pass out all the columns names from setString method right? And also I am not supposed to synchronize setStrings method here? –  Farhan Jamal Feb 9 '13 at 9:44
    
+1 for passing CallableStatement object as parameter.. –  Vishal K Feb 9 '13 at 10:44
up vote 1 down vote accepted

Following things are observed in your code:

  1. Constants is the name of class itself.
  2. getXXX method is static method in class Constants

Going by all above analysis I consider the use of Reflection API to call the getXXX methods of class Constants and storing those methods in an ArrayList. And finally calling these methods in a loop. The code look something like this:

class Task implements Runnable {

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

    public Task() {

    }
    public ArrayList<Method> getRequiredMethods()
    {
        Class<Constants> consClass = Constants.class;
        Method[] methods = consClass.getDeclaredMethods();
        ArrayList<Method> requiredMethods = new ArrayList();
        for (int i = 0 ; i < methods.length ; i++)
        {
            String sName = methods[i].getName();
            if (sName.startsWith("seta"))
            {
                requiredMethods.add(methods[i]);
            }
        }
        return requiredMethods;
    }

    @Override
    public void run() {
    try
    {
        dbConnection = getDbConnection();
        callableStatement = dbConnection.prepareCall(Constants.UPSERT_SQL);
        ArrayList<Method> methods = getRequiredMethods();
        callableStatement.setString(1 , String.valueOf(userId));
        for (int i = 0 ; i < methods.length ; i++)
        {
            //callableStatement.setString(i+2,(String)((methods.get(i)).invoke(null,userId)));
            methods.get(i).invoke(null,callableStatement,userId);
        }   
    }
    catch (Exception ex)
    {
        ex.printStackTrace();
    }
  }

And change your getxxx to setxxx method as follows:

 public static void setaAccount(final CallableStatement stat, int userId) {
        final String A_ACCOUNT = "{\"lv\":[{\"v\":{\"regSiteId\":null,\"userState\":null,\"userId\":" + userId
                + "},\"cn\":1}],\"lmd\":1360185069691}";
        stat.setString(2,A_ACCOUNT);//2 is the column Number.
    }
share|improve this answer
    
Thanks Vishal. Looks more cleaner now. I tried your suggestion but it is complaining at few lines like The method setString(int, String) in the type PreparedStatement is not applicable for the arguments (int, Object) and also at NameComprator as well it cannot be resolved something and also The method compare(Method, Method) of type XMPTask must override or implement a supertype method. –  Farhan Jamal Feb 9 '13 at 8:34
    
It was happening because (methods.get(i)).invoke(null,userid) returns Object. I have typecast it to String now . See the updated Code. –  Vishal K Feb 9 '13 at 8:38
    
Yeah that thing I figured it out. But what about other errors I mentioned? I think you forgot to put class name right? –  Farhan Jamal Feb 9 '13 at 8:41
    
I fixed that as well. Sorry for bothering. –  Farhan Jamal Feb 9 '13 at 8:44
    
It was Typo mistake . forget to write class before MyComparator declaration. It is fixed now –  Vishal K Feb 9 '13 at 8:45

Add a method in class Constants

public String getValue(int column,String userId) {
     // return appropriate value for column.
}

then use it

for( int i=1;i<=totalColumns;i++) {
   callableStatement.setString(i, constants.getValue(i,userId));   
}
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.