6
\$\begingroup\$

I have implemented an interface through which one can select, update and remove items of the Account class in a MySQL database. For the three functions I'm preparing statements.

The database, column and table names are all configured in a settings file.

Because of this I have to call the interface which gets the configuration from the settings file while preparing my query. This gives me a huge code block of code with little string concatenations in between:

updateStmt(connection->prepareStatement(
            "INSERT INTO " + GETSET(PTH_STORAGE_ACCOUNT
                                            PTH_PART_STORAGE_TABLE_NAME)
            + "(" + GETSET(PTH_STORAGE_SYSTEM_IDCOL) + ","
            + GETSET(PTH_STORAGE_ACCOUNT
                             PTH_PART_STORAGE_ACCOUNT_OWNERCOL) + ","
            + GETSET(PTH_STORAGE_ACCOUNT
                             PTH_PART_STORAGE_ACCOUNT_NAMECOL) + ","
            + GETSET(PTH_STORAGE_ACCOUNT
                             PTH_PART_STORAGE_ACCOUNT_DESCRIPTIONCOL) + ","
            + GETSET(PTH_STORAGE_ACCOUNT
                             PTH_PART_STORAGE_ACCOUNT_PASSWORDCOL) + ","
            + GETSET(PTH_STORAGE_ACCOUNT
                             PTH_PART_STORAGE_ACCOUNT_BALANCECOL) + ","
            + GETSET(PTH_STORAGE_ACCOUNT
                             PTH_PART_STORAGE_ACCOUNT_LOCKEDCOL) + ","
            + GETSET(PTH_STORAGE_ACCOUNT
                             PTH_PART_STORAGE_ACCOUNT_PASSWORDEDTRANSACTIONSCOL)
            + "," + GETSET(PTH_STORAGE_SYSTEM_CHANGEDCOL) + ","
            + GETSET(PTH_STORAGE_SYSTEM_CREATEDCOL) + ","
            + GETSET(PTH_STORAGE_SYSTEM_DELETEDCOL)
            + ") VALUES (?,?,?,?,?,?,?,?,NOW(),NOW(),NULL) ON DUPLICATE KEY UPDATE"
            + GETSET(PTH_STORAGE_ACCOUNT
                             PTH_PART_STORAGE_ACCOUNT_OWNERCOL) + "=VALUES("
            + GETSET(PTH_STORAGE_ACCOUNT
                             PTH_PART_STORAGE_ACCOUNT_OWNERCOL) + "), "
            + GETSET(PTH_STORAGE_ACCOUNT
                             PTH_PART_STORAGE_ACCOUNT_NAMECOL) + "=VALUES("
            + GETSET(PTH_STORAGE_ACCOUNT
                             PTH_PART_STORAGE_ACCOUNT_NAMECOL) + "), "
            + GETSET(PTH_STORAGE_ACCOUNT
                             PTH_PART_STORAGE_ACCOUNT_DESCRIPTIONCOL)
            + "=VALUES(" + GETSET(PTH_STORAGE_ACCOUNT
                                          PTH_PART_STORAGE_ACCOUNT_DESCRIPTIONCOL)
            + "), " + GETSET(PTH_STORAGE_ACCOUNT
                                     PTH_PART_STORAGE_ACCOUNT_PASSWORDCOL)
            + "=VALUES(" + GETSET(PTH_STORAGE_ACCOUNT
                                          PTH_PART_STORAGE_ACCOUNT_PASSWORDCOL)
            + "), " + GETSET(PTH_STORAGE_ACCOUNT
                                     PTH_PART_STORAGE_ACCOUNT_BALANCECOL)
            + "=VALUES(" + GETSET(PTH_STORAGE_ACCOUNT
                                          PTH_PART_STORAGE_ACCOUNT_BALANCECOL)
            + "), " + GETSET(PTH_STORAGE_ACCOUNT
                                     PTH_PART_STORAGE_ACCOUNT_LOCKEDCOL)
            + "=VALUES(" + GETSET(PTH_STORAGE_ACCOUNT
                                          PTH_PART_STORAGE_ACCOUNT_LOCKEDCOL)
            + "), " + GETSET(PTH_STORAGE_ACCOUNT
                                     PTH_PART_STORAGE_ACCOUNT_PASSWORDEDTRANSACTIONSCOL)
            + "=VALUES(" + GETSET(PTH_STORAGE_ACCOUNT
                                          PTH_PART_STORAGE_ACCOUNT_PASSWORDEDTRANSACTIONSCOL)
            + "), " + GETSET(PTH_STORAGE_SYSTEM_CHANGEDCOL) + "=NOW()"
            + GETSET(PTH_STORAGE_SYSTEM_DELETEDCOL) + "=NULL"
    ))

GETSET(x) is a define for settingsCtrl.readSetting(x) to shorten the call. PTH_STORAGE_... and PTH_PART_... are defines for the XPaths to the configuration value.

PTH_STORAGE_ACCOUNT PTH_PART_STORAGE_TABLE_NAME is an XPath to the account section and the configuration value for the table name.

I think it is obvious that building a query like this seems a bit wrong, but I'm not sure if there is a more concise way to prepare a query with so much parameters.

Is there?

\$\endgroup\$
  • \$\begingroup\$ May be you could split it up into different functions and concatenate the results? Also, if someone will want to concatenate another string literal in the beginning, it's gonna do something (not) funny. \$\endgroup\$ – Incomputable Mar 13 '17 at 17:30
  • \$\begingroup\$ @Incomputable I was thinking of this already, but I guess it won't reduce the above code very much. When I loop over the parameters I would have to introduce a lot of if-else-statements to consider the cases like =NOW(). Do you have something in mind? \$\endgroup\$ – Daniel Mar 14 '17 at 10:28
  • \$\begingroup\$ you could write some sophisticated iterator. Though I think it might escalate complexity quite a bit. Or you could use multiple iterators on different ranges. The key here is to understand the pattern in which those things come. If it is chaotic, then I believe there is no way to do anything about it. \$\endgroup\$ – Incomputable Mar 14 '17 at 10:30
2
\$\begingroup\$

Well to be blunt as it stands it is totally unreadable and thus unmaintainable.

You should break that up into multiple variables that are combined in a way that is readable.

std::stringstream columnName;
std::stringstream statement;

columnNames << storageAccount << ", "
            << storageName    << ", "
            << storageDesc    << ", "
            << storagePass    << ", "
            << storageBalance << ", "
            << storageLoc     << ", "
            << storageAccount << ", "
            << storageChanged << ","
            << storageDelCol;

statement << "INSERT INTO " << table 
          << "     (" << columnNames.str() << ") "
          << "     VALUES(?, ?, ?, ?, ?, ?, ?, ?, NOW(), NOW(), NULL) "
          << "ON DUPLICATE KEY  UPDATE "
          <<       storageAccount << " = VALUES(" << storageAccount << "), "
          <<       storageName    << " = VALUES(" << storageName    << "), "
          <<       storageDesc    << " = VALUES(" << storageDesc    << "), "
          <<       storagePass    << " = VALUES(" << storagePass    << "), "
          <<       storageBalance << " = VALUES(" << storageBalance << "), "
          <<       storageLoc     << " = VALUES(" << storageLoc     << "), "
          <<       storageAccount << " = VALUES(" << storageAccount << "), "
          <<       storageChanged << " = NOW(),"
          <<       storageDelCol  << " = NULL";
\$\endgroup\$
  • \$\begingroup\$ Although this is in principle the same approach as in my question (not reverting to introduce new functions/loops/...) it looks much cleaner and I see the gain in maintainability and readability. \$\endgroup\$ – Daniel Mar 15 '17 at 13:22

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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