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
| try (ZipFile zipFile = new ZipFile(file);) { | ||
| return zipFile.stream().map(entry -> getUri(file, entry)) // | ||
| .filter(Objects::nonNull).filter(isValidPredicate) // | ||
| .collect(Collectors.toSet()); |
There was a problem hiding this comment.
Another subtle change in behavior. Collectors.toSet doesn't make any API guarantees about mutability of the returned set. Regardless of my personal preference of iteration vs stream pipelines, I'd go for collect(Collectors.toCollection(HashSet::new))
There was a problem hiding this comment.
That's right, but since PathTraverser.traverseArchive() respectively findAllResourceUris() does not make any guarantees as well I assumed it's fine to change this implementation detail. However, I applied your suggestion.
| @@ -84,7 +84,7 @@ public String toString() { | |||
| result.append("(empty)"); | |||
| 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
from
b5cd50d
to
e43173b
Compare
HannesWell
force-pushed
the
internal-cleanups
branch
from
e43173b
to
8af825e
Compare
|
Just rebased this on the latest master, is there anything else I should adjust? |
This PR contains the internal code simplifications and modernizations that happened as side effect of the following PRs, in order to ease their review: