Java efficiency in for loops

Question

I have two implementations of the same functionality they look as follows.

private String mapToString(Map<String, String> map) 
        throws UnsupportedEncodingException
{
    if(map.isEmpty())
    {
        throw new IllegalArgumentException("Map cannot be empty");
    }

    StringBuilder sb = new StringBuilder();

    for(Map.Entry<String, String> pair : map.entrySet())
    {
        if(pair.getKey() == null)
        {throw new IllegalArgumentException("Invalid parameter:" +
                " Parameters key is null");}

        if(pair.getValue() == null)
        {throw new IllegalArgumentException("Invalid parameter:" +
                " Parameters value is null");}


        // Because the map cannot be empty and can be of arbitrary size it
        // is it more efficient to append the _ at the end of each cycle and 
        // remove the extra _ when the string is done being built. 

        sb.append(URLEncoder.encode(pair.getKey(), "UTF-8"));
        sb.append('-');
        sb.append(URLEncoder.encode(pair.getValue(), "UTF-8"));
        sb.append('_');
    }

    String result = sb.toString();

    // Remove the extra _
    return result.substring(0, result.length() - 1);
}

and the second one

private String mapToString(Map<String, String> map) 
        throws UnsupportedEncodingException
{
    if(map.isEmpty())
    {
        throw new IllegalArgumentException("Map cannot be empty");
    }

    StringBuilder sb = new StringBuilder();

    for(Map.Entry<String, String> pair : map.entrySet())
    {
        if(pair.getKey() == null)
        {throw new IllegalArgumentException("Invalid parameter:" +
                " Parameters key is null");}

        if(pair.getValue() == null)
        {throw new IllegalArgumentException("Invalid parameter:" +
                " Parameters value is null");}

        if(sb.length() != 0)
        {
             sb.append('_');
        }   
        sb.append(URLEncoder.encode(pair.getKey(), "UTF-8"));
        sb.append('-');
        sb.append(URLEncoder.encode(pair.getValue(), "UTF-8"));
    }

    return sb.toString();
}

Each version of this method takes a map of strings that is guaranteed to not be empty and creates a string from them. The string starts with the key followed by a - then the value. Each of these pairs are separated by a _.

key1-value1_key2-value2_key3-value3_......

My two options are to check the to see if string is empty and not place the _ separator and save myself creating a substring at the end. Or do not do a check, append the _ at the end of the cycle and then use a substring to remove the extra _ that results.

I am curious as to which would be more efficient for an arbitrarily large map.

Premature optimization? Usually when your code is running slow, improvement requires a fundamental algorithm change, not a small one like this. And optimizations like this are only useful to consider *when* your code is running slow- not before. — William Morrison, Jul 30 '13 at 19:33
You could make a very large map and [try it yourself](http://stackoverflow.com/questions/180158/how-do-i-time-a-methods-execution-in-java). — ajp15243, Jul 30 '13 at 19:33
It probably is premature optimisation, however both are very readable so I don't see an issue trying to use the most efficient version. — assylias, Jul 30 '13 at 19:41
While this is about optimization, it is more about an academic interest and writing efficient code the first time. While I understand that premature optimization is bad, I feel that it is sloppy to knowingly write inefficient code just because hardware speed can hide the flaws. — JME, Jul 30 '13 at 19:43
It is best to test it but second version is most likely quicker as the processor will be able to do some efficient branch prediction and the if should quickly become free. Even faster (possibly) would be to use a boolean `firstIteration` instead of the int comparison. You could also get rid of the check on the keys with (before the loop) `if (map.containsKey(null)) ...`. — assylias, Jul 30 '13 at 19:43
`StringBuilder` also has a `setLength` method you can use to strip the last char off. Anyway, in practice I'd use #2 just because it is more readable. BTW `URLEncoder` does not encode dashes or underscores, so not sure if you want to apply this (irreversible) encoding in the loop or once afterwards — mihi, Jul 30 '13 at 19:46
@mihi You are correct, the encoding can be done once the string is built. Thanks for pointing that out. — JME, Jul 30 '13 at 19:58

arshajii · Accepted Answer · 2013-07-30T19:42:56.550

The deciding factor between those two implementations should really come down to readability. Having said that, your first implementation that uses substring will be more expensive, and since I see no readability difference between the two, I would prefer the second variant. An alternative would simply be to use a boolean flag instead of checking sb.length():

append = false;

for (...) {
    ...

    if (append) {
        sb.append('_');
    }

    ...

    append = true;
}

Java efficiency in for loops

1 Answers1