Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

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

The 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.

Are there any way to make this look cleaner, keeping in mind thread safety issues?

class Task implements Runnable {

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

    public Task() {


    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;

      dbConnection = DriverManager.getConnection(url,user,password);

      return dbConnection;

This is 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.awaitTermination(Long.MAX_VALUE, TimeUnit.SECONDS);

    while (!service.isTerminated()) {

} catch (Exception e) {

share|improve this question

migrated from 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

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 (
    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
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"))
        return requiredMethods;

    public void run() {
        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++)
    catch (Exception ex)

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

Your Answer


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