HiveBrain v1.2.0
Get Started
← Back to all entries
snippetjavaMinor

How to make an iteration in a for-loop faster?

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
loopmakeiterationfasterforhow

Problem

I have a list with contentlets and want all languages from every contentlet (I get these with the method languageAPI.getAllValuesByKey(key , contentList). I get a HashMap and iterate over it. There are 1000 keys available. In the beginning it only takes 2 ms per key. But after a while it increases. And the last one takes 35ms. How can I decrease these times? How can I make it faster/more efficient?

JSONArray arrAll = new JSONArray();
JSONObject jsonObject = new JSONObject();
JSONArray arr = new JSONArray();
JSONObject values = new JSONObject();

List contentlets = languageFactory.getAllContentlets(null);
List keys = languageAPI.getLanguageKeys(null, contentlets);

for(Contentlet key : keys) {
    Long startTime = System.currentTimeMillis();
    jsonObject = new JSONObject();

    HashMap allValues = languageAPI.getAllValuesByKey(key.getStringProperty("key"), contentlets);
    Iterator> it = allValues.entrySet().iterator();
    arr = new JSONArray();
    while (it.hasNext()) {
        values = new JSONObject();
        java.util.Map.Entry pairs = it.next();
        values.put("l", pairs.getKey());
        values.put("v", pairs.getValue());
        arr.add(values);
        it.remove(); // avoids a ConcurrentModificationException
    }

    try {
        jsonObject.put("k", key.getStringProperty("key"));
        jsonObject.put("t", (Object)arr);
        jsonObject.put("p", key.isLive());
        jsonObject.put("l", key.isLocked());
        jsonObject.put("a", key.isArchived());
    } catch (DotStateException e) {
        throw new RuntimeException(e.toString(),e);
    } catch (DotDataException e) {
        throw new RuntimeException(e.toString(),e);
    } catch (DotSecurityException e) {
        throw new RuntimeException(e.toString(),e);
    }

    arrAll.add(jsonObject);
    Long end = System.currentTimeMillis() - startTime;
    Logger.info(this, "For key: " + key.getStringProperty("key") + " " + end  + "ms");
}


Edit:

One of the problems is that th

Solution

-
getStringKey iterates over the contentlets list for every key. It looks O(n^2) which does not scale well.
You could create a HashMap cache with a proper hashCode and equals for ContentletCacheKey and use this instead of iterating over the list every time.

-
Try to minimize the scope of local variables. It's not necessary to declare them at the beginning of the method, declare them where they are first used. (Effective Java, Second Edition, Item 45: Minimize the scope of local variables)

This object creation is unnecessary since you create a new object at the beginning of the for loop:

JSONObject jsonObject = new JSONObject();


The same is true for values and arr.

-
Your statistics log could show invalid data because if you change the system date System.currentTimeMillis() will reflect that. I suggest using System.nanoTime() or a stopwatch class (Guava, Apache Commons).

-
public String getStringKey(Long languageId, String key, 
        List contentlets) {
    String value = null;

    for (final Contentlet keyEntry: contentlets) {
        if (keyEntry.getStringProperty("key").equals(key) 
                && languageId == keyEntry.getLanguageId()) {
            return keyEntry.getStringProperty("value");
        }
    }

    if (value == null) {
        value = "";
    }
    return value;
}


This could be more simple:

public String getStringKey(Long languageId, String key, 
        List contentlets) {
    for (final Contentlet keyEntry: contentlets) {
        if (keyEntry.getStringProperty("key").equals(key) 
                && languageId == keyEntry.getLanguageId()) {
            return keyEntry.getStringProperty("value");
        }
    }
    return "";
}


-
Iterator> it 
        = allValues.entrySet().iterator();
    JSONArray arr = new JSONArray();
    while (it.hasNext()) {
        JSONObject values = new JSONObject();
        java.util.Map.Entry pairs = it.next();
        values.put("l", pairs.getKey());
        values.put("v", pairs.getValue());
        arr.add(values);
        it.remove(); // avoids a ConcurrentModificationException
    }


If I'm right you can clear the the whole map after loop (instead of remove in every iteration) and could use a foreach loop:

for (final Map.Entry pairs: allValues.entrySet()) {
    final JSONObject values = new JSONObject();
    values.put("l", pairs.getKey());
    values.put("v", pairs.getValue());
    arr.add(values);
}
allValues.clear();


Actually, the clear() need unnecessary (as well as the it.remove()) since you don't use the allValues map after the while (for) loop. Furthermore, importing java.util.Map.Entry makes the code easier to read.

-
HashMap reference types should be simply Map. See: Effective Java, 2nd edition, Item 52: Refer to objects by their interfaces

Code Snippets

JSONObject jsonObject = new JSONObject();
public String getStringKey(Long languageId, String key, 
        List<Contentlet> contentlets) {
    String value = null;

    for (final Contentlet keyEntry: contentlets) {
        if (keyEntry.getStringProperty("key").equals(key) 
                && languageId == keyEntry.getLanguageId()) {
            return keyEntry.getStringProperty("value");
        }
    }

    if (value == null) {
        value = "";
    }
    return value;
}
public String getStringKey(Long languageId, String key, 
        List<Contentlet> contentlets) {
    for (final Contentlet keyEntry: contentlets) {
        if (keyEntry.getStringProperty("key").equals(key) 
                && languageId == keyEntry.getLanguageId()) {
            return keyEntry.getStringProperty("value");
        }
    }
    return "";
}
Iterator<java.util.Map.Entry<Long, String>> it 
        = allValues.entrySet().iterator();
    JSONArray arr = new JSONArray();
    while (it.hasNext()) {
        JSONObject values = new JSONObject();
        java.util.Map.Entry<Long, String> pairs = it.next();
        values.put("l", pairs.getKey());
        values.put("v", pairs.getValue());
        arr.add(values);
        it.remove(); // avoids a ConcurrentModificationException
    }
for (final Map.Entry<Long, String> pairs: allValues.entrySet()) {
    final JSONObject values = new JSONObject();
    values.put("l", pairs.getKey());
    values.put("v", pairs.getValue());
    arr.add(values);
}
allValues.clear();

Context

StackExchange Code Review Q#20992, answer score: 7

Revisions (0)

No revisions yet.