3
\$\begingroup\$

I have this piece of Java (Android) code that adds a list of brokers to a local SQLite database as one single SQL instruction.

public void Add(List<Broker> brokers)
{
    if(brokers == null || brokers.size() == 0)
      return;

    String sql = "INSERT INTO " + TABLE_NAME + " SELECT " + brokers.get(0).getId() + " AS '" + COLUMN_BROKERID + "', "+ brokers.get(0).getOfficeId() + " AS '" + COLUMN_OFFICEID +  "', '"+ brokers.get(0).getName() + "' AS '" + COLUMN_NAME +  "', "+ brokers.get(0).getSuccessRate() + " AS '" + COLUMN_SUCCESSRATE +  "'";

    for(int i=1; i<brokers.size(); i++)
    {
        sql = sql + " UNION SELECT " + brokers.get(i).getId() + ", " + brokers.get(i).getOfficeId() + ", '" + brokers.get(i).getName() + "', " + brokers.get(i).getSuccessRate();
    }

    databaseManager.ExecuteNonQuery(sql);
 }

But what slows this down a lot is the change in the string 'sql'. The last line, which is a call to ExecuteNonQuery(), takes a millisecond, but the above takes a lot. How can I speed this up?

databaseManager is an interface that is implemented by a class of SQLiteOpenHelper and simply executes an SQL statement (but don't take this into account; it doesn't take any time as much as the rest of the code).

\$\endgroup\$
  • \$\begingroup\$ Add some line breaks ;) \$\endgroup\$ – mheinzerling Apr 11 '13 at 8:04
4
\$\begingroup\$

A classicel one. String concatenation with the + operator is very inefficient, because it creates a new copy of the string every time.
See for example item 51 in Effective Java from Joshua Bloch or here: http://www.javapractices.com/topic/TopicAction.do?Id=4

Try to change it to StringBuilder:

public void Add(List<Broker> brokers)
{
    if(brokers == null || brokers.size() == 0)
      return;

    StringBuilder query = new StringBuilder(brokers.size() * 50); // size is a guess, could be furhter investigated
    query.append("INSERT INTO ").append(TABLE_NAME).append(" SELECT ").append(brokers.get(0).getId()).append(" AS '").append(COLUMN_BROKERID).append("', ").append(brokers.get(0).getOfficeId()).append(" AS '").append(COLUMN_OFFICEID).append("', '").append(brokers.get(0).getName()).append("' AS '").append(COLUMN_NAME).append("', ").append(brokers.get(0).getSuccessRate()).append(" AS '").append(COLUMN_SUCCESSRATE).append("'"); // it is not that important to change this line. this will probably be done by the java compiler anyway

    for(int i=1; i<brokers.size(); i++)
        query.append(" UNION SELECT ").append(brokers.get(i).getId()).append(", ").append(brokers.get(i).getOfficeId()).append(", '").append(brokers.get(i).getName()).append("', ").append(brokers.get(i).getSuccessRate());

    databaseManager.ExecuteNonQuery(query.toString(););
 }
\$\endgroup\$
  • 1
    \$\begingroup\$ Actually the compiler is optimizing the + to StringBuilder in some cases. So I prefer the + outside of the loop or local in the loop for readability and only use the append between the loop iteration. In most common cases readability is more worth than some mircoseconds, especially as the SQL/Netweork is the slowest part here. \$\endgroup\$ – mheinzerling Apr 11 '13 at 8:02
  • \$\begingroup\$ Just scrolled to the very right and found your comment stating the same ;) \$\endgroup\$ – mheinzerling Apr 11 '13 at 8:04
  • \$\begingroup\$ Yes, I agree, readability should always be the first main goal. I do not have a clear opinion what is the best way for mixing string with variables. We have +, append, String.format, one line, multiple lines, and so on. For readability, I like the possibility from php to just use a variable inside the string. \$\endgroup\$ – tb- Apr 11 '13 at 9:03
0
\$\begingroup\$

This is my take. Longer but separates each concern, making updating/investigating the SQL statement itself easier.

private final static String SQL_TEMPLATE = "INSERT INTO {0} SELECT {1} AS '{2}'," +
           " {3} AS '{4}', '{5}' AS '{6}', {7} AS '{8}'";
private final static String UNION_TEMPLATE = " UNION SELECT {0}, {1}, '{2}', {3}";

public void add(List<Broker> brokers) {
    // consider using Apache Commons StringUtils.isBlank(str):
    // catches empty and null Strings
    if (brokers == null || brokers.isEmpty()) {
        return;
    }

    Broker firstBroker = broker.get(0);
    String sql = java.text.MessageFormat.format(
            SQL_TEMPLATE,
            TABLE_NAME,
            firstBroker.getId(),
            COLUMN_BROKERID,
            firstBroker.getOfficeId(),
            COLUMN_OFFICEID,
            firstBroker.getName(),
            COLUMN_NAME,
            firstBroker.getSuccessRate(),
            COLUMN_SUCCESSRATE);

    for (int i = 1; i < brokers.size(); i++) {
        Broker nextBroker = brokers.get(i);

        String unionClause = java.text.MessageFormat.format(UNION_TEMPLATE,
                                    nextBroker.getId(),
                                    nextBroker.getOfficeId(),
                                    nextBroker.getName(),
                                    nextBroker.getSuccessRate());
        sql.concat(unionClause);
    }

    databaseManager.executeNonQuery(sql);
}
  1. Java Naming conventions: method names should start with a lowercase letter ("Add" -> "add", "Execute" -> "execute"
  2. Use all methods API gives you, for example List.isEmpty() instead of List.size() == 0 to improve readability / expressiveness.
  3. Since TABLE_NAME, COLUMN_BROKERID, etc. appear to be constants I'd include them in the SQL template to decrease number of required replaces and improve readability.
  4. Since there are so many parameters in each SQL you could consider using a different mechanism that supports named parameters (for example :id, :broker rather than ordered like {0}, {1} etc.)
  5. I wouldn't worry about String concatenation performance as it's not likely to be the bottleneck; calling the DB will be orders of magnitue more time-consuming,
  6. Doesn't SQLite support PreparedStatements? Is SQL injection a viable threat in your application?

Update: sorry, I didn't notice your "takes a millisecond" remark; to be honest I find it hard to believe that String concatenation (even for multiple loop iterations) could take significantly more time that a DB update; is your profiling methodology 100% reliable?

\$\endgroup\$

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.