woj-tek commented on code in PR #2386:
URL: https://github.com/apache/james-project/pull/2386#discussion_r1726683857
##########
protocols/imap/src/main/java/org/apache/james/imap/message/response/ImmutableStatusResponse.java:
##########
@@ -72,12 +70,22 @@ public ImapCommand getCommand() {
@Override
public String toString() {
- return MoreObjects.toStringHelper(this)
- .add("responseCode", responseCode)
- .add("serverResponseType", serverResponseType)
- .add("tag", tag)
- .add("textKey", textKey)
- .add("command", command)
- .toString();
+ final StringBuilder sb = new StringBuilder();
+ if (getResponseCode() != null) {
+ sb.append(' ').append(getResponseCode());
+ }
+ if (getServerResponseType() != null) {
+ sb.append(' ').append(getServerResponseType());
+ }
+ if (getTag() != null) {
+ sb.append(' ').append(getTag());
+ }
+ if (getTextKey() != null) {
+ sb.append(' ').append(getTextKey());
+ }
+ if (getCommand() != null) {
+ sb.append(' ').append(getCommand());
+ }
+ return sb.toString();
Review Comment:
> In non critical code portions or non costly operations we don't really
care but for big string concatenations this might be a concern.
Indeed, but this is not-user facing and mostly for debugging so it shouldn't
be that visible in production?
But I agree that making it more optimal is always better.
> If wrapping things in ifs is for you also not your cup of tea we
apparently can use suppliers in the logging event builder (I just found this
out!)
Yeah, slf4j has awesome fluent API since 2.x.
Though in this case we are not using logger but having/discussing actual
(`.toString()`) method which this doesn't apply directly. And if/when called we
don't really need to use fluent API because object would be passed directly to
slf4j and if logging is not enabled then `toString()` would not be called
(beauty of slf4j api, even not fluent -- and the optimizations for 1/2/3
arguments instead of using varargs always which yields pesky array allocation
under the hood :) ).
Also, JID should be smart enough to optimise those pesky ifs IMHO.
On the other hand, looking at `MoreObjects` - it does have interesing effect
of allocating new `ValueHolder` for each added field... and using
`StringBuilder` and null-check-with-ifs in it's `toString()`. While allocation
is cheap it affects overall GC. And considering
`MoreObjects.ToStringHelper#toString` implementation one could argue that using
simpler, hand-crafted toString that performs less checks could be more
efficient ;)
At any rate - I reverted the `toString` to original flavour and moved custom
one to `asString`, though considering removal of logging in
`UnpooledStatusResponseFactory` it doesn't matter that much (at least for me,
logging in `ChannelImapResponseWriter` is perfect!)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
