feat: improve toString()
#208
Conversation
ppkarwasz
left a comment
ppkarwasz
left a comment
There was a problem hiding this comment.
The replacement of String with char looks good to me!
I don't understand the other changes.
| purl.append("?"); | ||
| qualifiers.forEach((key, value) -> { | ||
| purl.append(toLowerCase(key)); | ||
| purl.append("="); | ||
| purl.append(percentEncode(value)); | ||
| purl.append("&"); | ||
| }); | ||
| purl.setLength(purl.length() - 1); | ||
| purl.append('?'); | ||
| Iterator<Map.Entry<String, String>> it = qualifiers.entrySet().iterator(); | ||
| Map.Entry<String, String> entry = it.next(); | ||
| purl.append(entry.getKey()); | ||
| purl.append('='); | ||
| purl.append(percentEncode(entry.getValue())); | ||
| while (it.hasNext()) { | ||
| purl.append('&'); | ||
| entry = it.next(); | ||
| purl.append(toLowerCase(entry.getKey())); | ||
| purl.append('='); | ||
| purl.append(percentEncode(entry.getValue())); | ||
| } |
There was a problem hiding this comment.
Why this big change?
Map.forEach is garbage-free, while using an iterator isn't.
There was a problem hiding this comment.
I am not sure which part is the "garbage". It seemed like a bit of a hack to write an extra '&' and then delete it by filling the rest of the buffer with '\0'. I thought this was better as it just writes the exact number. We know we must have at least one pair at at that point.
I feel like something like
qualifiers.stream().entrySet().stream().map(Object::toString).collect(Collectors.joining("&"));looks even "cleaner", but requires multiple buffers. I guess the trick is to do it using a single buffer.
But, I could revert it. I don't care that much.
There was a problem hiding this comment.
By "garbage-free" I mean that it does not create any temporary Java objects (except those created by toLowerCase and percentEncode).
The new code creates an iterator and map entries.
There was a problem hiding this comment.
I see, you meant "garbage" as in "garbage collection", but Map::forEach does create an entrySet already. Point taken about the iterator, though. It's possible to remove the iterator.
There was a problem hiding this comment.
Map.forEach is not GC-free but some implementations (e.g. HashMap) are.
There was a problem hiding this comment.
Yes, it seems there is no entrySet in TreeMap until you create it. In that case, validateQualifiers() should remove its use too?
There was a problem hiding this comment.
My rule of thumb is that if it is not too much trouble, GC-free code should be preferred. If it makes code much more complex, then more modern constructs are preferred.
As a curiosity IDEA always suggests using forEach() instead of stream().forEach()
There was a problem hiding this comment.
@ppkarwasz It looks like it was merged anyway. If you want, make a pull request to revert that part.
I think forEach() is potentially faster because it avoids the stream() call/creation. I can't think of a case where the stream would be better since it's a terminal operation, anyway, so you can't do anything with the stream.
There was a problem hiding this comment.
@ppkarwasz Since the entrySet may have been created already in a method prior to the toString() call, I think it exists for the lifetime of the object, and there should be no additional overhead (memory or time) to using it in toString().
dwalluck
force-pushed
the
to-string
branch
5 times, most recently
from
9402e82 to
bf921b5
Compare
* Use single character append instead of string append * Avoid `StringBuilder::setLength` for qualifiers * Qualifier keys are already lowercase
3 participants
StringBuilder::setLengthfor qualifiers