Various internal code simplifications and modernizations #2989
Conversation
HannesWell
force-pushed
the
internal-cleanups
branch
from
c67352b to
7dab591
Compare
org.eclipse.xtext/src/org/eclipse/xtext/mwe/RuntimeResourceSetInitializer.java
Outdated
Show resolved
Hide resolved
HannesWell
force-pushed
the
internal-cleanups
branch
from
7dab591 to
b5cd50d
Compare
| else { | ||
| result.append("{\n "); | ||
| result.append(Joiner.on("\n ").join(uris)); | ||
| result.append(uris.stream().map(URI::toString).collect(Collectors.joining("\n "))); |
There was a problem hiding this comment.
Quite a mouthful compared to Joiner.on. Both in terms of characters, cognitive load and object allocations. I'd keep this as a Joiner.
There was a problem hiding this comment.
Quite a mouthful compared to Joiner.on. Both in terms of characters, cognitive load and object allocations.
Yes it has more characters, but personally I would say it is simple since you can read the pipeline from left to right, but I'm more used to Streams than Guava's Joiner so it is a matter of taste.
I haven't counted all object creations, but Joiner also creates objects and the JVM+GCs are usually good in handling short living objects respectively even avoiding their creation on the heap.
But the biggest impact is probably due to the creation of the String result of the join operation and if you are really concerned about object creation here, I would try to avoid that by using something like the following instead:
for (Iterator<String> containers = invisibleContainers.iterator(); containers.hasNext();) {
result.append(containers.next());
if(containers.hasNext()) {
result.append("\n ");
}
}
That being said, I reverted that change for now.
HannesWell
force-pushed
the
internal-cleanups
branch
2 times, most recently
from
e43173b to
8af825e
Compare
|
Just rebased this on the latest master, is there anything else I should adjust? |
HannesWell
force-pushed
the
internal-cleanups
branch
from
8af825e to
0b4bcac
Compare
| */ | ||
| public static <T> Multimap<T,IEObjectDescription> index(Iterable<IEObjectDescription> descriptions, Function<IEObjectDescription,T> indexer) { | ||
| ArrayList<IEObjectDescription> list = Lists.newArrayList(descriptions); | ||
| List<IEObjectDescription> list = new ArrayList<>(); |
There was a problem hiding this comment.
It's ok here, but keep in mind that Lists.newArrayList(Iterable) has a special casing for Collections and will call the more optimal ArrayList(Collection) constructor, if possible.
There was a problem hiding this comment.
Yes, that's right. But since the purpose of the entire creation of the ArrayList seems to be being able to determine the size of the Iterable and at the same time avoiding the second pass over the Iterable for the subsequent iteration, the best would be to to test if the Iterable is collection and act accordingly or provide an overload for collections.
The latter could then be called by the current method after the iterable content was copied into a List.
2 participants
This PR contains the internal code simplifications and modernizations that happened as side effect of the following PRs, in order to ease their review: