Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 1 down vote favorite

So I have this piece of code in Java(Android) to add 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 take 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 doesnt take any time as much as the rest of the code)

share|improve this question
Add some line breaks ;) – mnhg Apr 11 at 8:04

2 Answers

up vote 3 down vote accepted

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(););
 }
share|improve this answer
1  
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. – mnhg Apr 11 at 8:02
Just scrolled to the very right and found your comment stating the same ;) – mnhg Apr 11 at 8:04
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. – tb- Apr 11 at 9:03
up vote 0 down vote

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?

share|improve this answer

Your Answer

 
discard

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

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