It’s amazing how many times I find myself violating my own rules, my own guidelines, the things I’ve preached over the years. Performance is often one of them. Knowing performance to be critical when it comes to database access, I’ve prematurely optimized one of the String utility methods. One of my invisible pairs (people commenting on these blogs) pointed out that my use of StringBuilder has unnecessarily obfuscated code in a couple spots.

StringUtil

    package util;
    
    public class StringUtil {
       public static String commaDelimit(String[] strings, Transformer transformer) {
          StringBuilder builder = new StringBuilder();
          for (int i = 0; i < strings.length; i++) {
             if (i > 0)
                builder.append(',');
             builder.append(transformer.transform(strings[i]));
          }
          return builder.toString();
       }
    
       public static String commaDelimit(String[] strings) {
          final Transformer nullTransform = new Transformer() {
             public String transform(String input) {
                return input;
             }
          };
          return commaDelimit(strings, nullTransform);
       }
    
       public static String wrap(String source, char wrapper) {
          if (source == null)
             return null;
          StringBuilder builder = new StringBuilder();
          builder.append(wrapper);
          builder.append(source);
          builder.append(wrapper);
          return builder.toString();
       }
    }

The wrap and commaDelimit methods could both be written using String concatenation (+) instead of StringBuilder. Habitually, I coded this using a StringBuilder. Now, I’ve preached against this unnecessary optimization for years, including in Essential Java Style. So why did I violate it?

If I were in court, I’d plead nolo contendere. The wrap method is far more succinctly written as:

       public static String wrap(String source, char wrapper) {
          if (source == null)
             return null;
          return wrapper + source + wrapper;
       }

Using a StringBuilder in this method significantly detracted from its readability. Guilty as charged. With respect to thecommaDelimit method, I plead legitimate past experience. The general rule of thumb is that String concatenation is fine when putting together a finite, reasonable number of Strings. When using looping constructs, where each iteration through the loop appends to the previous complete string (which starts empty), the performance degradation can be significant.

But really, you don’t know until you’ve measured it. The rule of performance is to only optimize after you’ve gotten things working properly, and only then if you need to. I do counterbalance that with a second rule: it’s ok to optimize if it does not detract from the readability or future understanding of the code. By “future understanding,” I’m talking about when someone looks at the code and has to say “why the heck would he do it that way?.” That’s not a good thing.

Consider a rewrite of the commaDelimit method:

       public static String commaDelimit(String[] strings, Transformer transformer) {
          String builder = "";
          for (int i = 0; i < strings.length; i++) {
             if (i > 0)
                builder += ',';
             builder += transformer.transform(strings[i]);
          }
          return builder.toString();
       }

In my humble opinion, that adds virtually nothing to its readability. Replacing a method call with a symbol is nice, but doesn’t really change much with respect to the time required to comprehend commaDelimit.

For now, I’m going to straddle the fence. I’ve already rewritten the wrap method. I intend on keeping the existing definition ofcommaDelimit, which, by the way, has another potential performance issue in the callback to the transformer. Performance debates never end. The best way to shut them off is to write a quick performance test. If someone else is concerned about performance, let me know, and we’ll tackle that.

ps – I did write the performance tests for commaDelimit. It’s from three to ten times slower when written using String concatenation. I also wrote the tests for wrap. It’s not quite twice as slow now, and I had to ratchet up the number of iterations to 100,000 before it even got into the realm of milliseconds. Right now, these measurements are meaningless.

Comments

Anonymous October 27, 2005 at 09:08am

Using buffer/builders for looping constructs is what I would have done as well.

–JeffBay

Share your comment