Skip to content
Closed
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@
import org.jspecify.annotations.Nullable;

import org.springframework.util.Assert;
import org.springframework.util.StringUtils;

/**
* Represents a SockJS frame. Provides factory methods to create SockJS frames.
Expand Down Expand Up @@ -83,7 +82,6 @@ else if (content.charAt(0) == 'c') {
}
}


/**
* Return the SockJS frame type.
*/
Expand Down Expand Up @@ -119,7 +117,6 @@ public byte[] getContentBytes() {
}
}


@Override
public boolean equals(@Nullable Object other) {
return (this == other || (other instanceof SockJsFrame that &&
Expand All @@ -133,15 +130,27 @@ public int hashCode() {

@Override
public String toString() {
String result = this.content;
if (result.length() > 80) {
result = result.substring(0, 80) + "...(truncated)";
int maxLen = 80;
int contentLength = this.content.length();
int len = Math.min(contentLength, maxLen);

StringBuilder sb = new StringBuilder(len + 20);

for (int i = 0; i < len; i++) {
char c = this.content.charAt(i);
switch (c){
case '\n' -> sb.append("\\n");
case '\r' -> sb.append("\\r");
default -> sb.append(c);
}
}
result = StringUtils.replace(result, "\n", "\\n");
result = StringUtils.replace(result, "\r", "\\r");
return "SockJsFrame content='" + result + "'";
}

if (contentLength > maxLen) {
sb.append("...(truncated)");
}

return "SockJsFrame content='" + sb + "'";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not use the String Builder for this part as well? This PR is trying to optimize for memory allocation after all.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the review.

Regarding the other toString() methods, they also use StringBuilder, so I thought there’s no need for the unnecessary String objects to become GC targets in the JVM. Of course, I agree that the original code is more readable.

My perspective is that the readability of this part isn’t particularly high, and concatenating immutable Strings in this way is generally considered an anti-pattern, so I opted to use a mutable StringBuilder.

I will revise the return part as you suggested.

@bclozel

}

public static SockJsFrame openFrame() {
return OPEN_FRAME;
Expand Down