snippetjavaMinor
How to make an iteration in a for-loop faster?
Viewed 0 times
loopmakeiterationfasterforhow
Problem
I have a list with
Edit:
One of the problems is that th
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
-
You could create a
-
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:
The same is true for
-
Your statistics log could show invalid data because if you change the system date
-
This could be more simple:
-
If I'm right you can clear the the whole map after loop (instead of
Actually, the clear() need unnecessary (as well as the
-
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 interfacesCode 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.