<Swing Dev> Replace concat String to append in StringBuilder parameters
Pavel Rappo
pavel.rappo at oracle.com
Mon Aug 11 14:33:14 UTC 2014
Otavio,
Just skimmed through your changes. It looks good. But there are some things we can make a little bit better though. IMO, it's not always a performance that matters (looking around to see if Alexey Shipilev is somewhere near) but readability. It's good to estimate performance requirements of a code before making a decision of whether to sacrifice readability for performance or not. I noticed there are some methods that are screaming for readability improvements. Namely, different versions of joining strings through a separator. You can find them in following methods in your changeset:
sun.security.acl.AclEntryImpl::toString
sun.security.ssl.HandshakeMessage.CertificateRequest::print
javax.swing.MultiUIDefaults::toString (one of the most bizarre one)
sun.security.provider.PolicyFile::expandSelf
java.security.ProtectionDomain::toString
javax.management.relation.Role::toString
sun.security.ssl.SupportedEllipticCurvesExtension::toString
sun.tools.jstat.SyntaxException::SyntaxException
Unfortunately, neither java.util.StringJoiner nor String.join have perfect (but who has?) APIs. So it's up to us to figure out the best way of joining elements. So when the power of those guys is not enough, we can go this way (or maybe some else):
Iterable<?> elements = ...
StringJoiner j = new StringJoiner(", ");
elements.forEach(e -> j.add(e.toString()));
or
Collection<?> elements = ...
String i = elements.stream()
.map(Object::toString)
.collect(Collectors.joining(", "));
Other than that:
1. It looks like some StringBuffers are still lurking out there. We should change them to StringBuilders, *only if* we can prove they are not exposed to more than one thread:
--- dev/jdk/src/share/classes/javax/sound/sampled/AudioFileFormat.java (revision 10452:8e501e6bbf1fda1bf9b19bc00fff02074162946f)
+++ dev/jdk/src/share/classes/javax/sound/sampled/AudioFileFormat.java (revision 10452+:8e501e6bbf1f+)
@@ -272,7 +272,7 @@
@Override
public String toString() {
- StringBuffer sb = new StringBuffer();
+ StringBuilder sb = new StringBuilder();
//$$fb2002-11-01: fix for 4672864: AudioFileFormat.toString() throws unexpected NullPointerException
if (type != null) {
--- dev/jdk/src/share/classes/com/sun/tools/hat/internal/model/JavaValueArray.java (revision 10452:8e501e6bbf1fda1bf9b19bc00fff02074162946f)
+++ dev/jdk/src/share/classes/com/sun/tools/hat/internal/model/JavaValueArray.java (revision 10452+:8e501e6bbf1f+)
@@ -346,12 +346,12 @@
public String valueString(boolean bigLimit) {
// Char arrays deserve special treatment
- StringBuffer result;
+ StringBuilder result;
byte[] value = getValue();
int max = value.length;
byte elementSignature = getElementType();
if (elementSignature == 'C') {
- result = new StringBuffer();
+ result = new StringBuilder();
for (int i = 0; i < value.length; ) {
char val = charAt(i, value);
result.append(val);
@@ -362,7 +362,7 @@
if (bigLimit) {
limit = 1000;
}
- result = new StringBuffer("{");
+ result = new StringBuilder("{");
int num = 0;
for (int i = 0; i < value.length; ) {
if (num > 0) {
--- dev/jdk/src/share/classes/javax/swing/GroupLayout.java (revision 10452:8e501e6bbf1fda1bf9b19bc00fff02074162946f)
+++ dev/jdk/src/share/classes/javax/swing/GroupLayout.java (revision 10452+:8e501e6bbf1f+)
@@ -1213,7 +1213,7 @@
registerComponents(horizontalGroup, HORIZONTAL);
registerComponents(verticalGroup, VERTICAL);
}
- StringBuffer buffer = new StringBuffer();
+ StringBuilder buffer = new StringBuilder();
buffer.append("HORIZONTAL\n");
createSpringDescription(buffer, horizontalGroup, " ", HORIZONTAL);
buffer.append("\nVERTICAL\n");
@@ -1221,7 +1221,7 @@
return buffer.toString();
}
- private void createSpringDescription(StringBuffer buffer, Spring spring,
+ private void createSpringDescription(StringBuilder buffer, Spring spring,
String indent, int axis) {
String origin = "";
String padding = "";
--- dev/jdk/src/share/classes/sun/net/spi/nameservice/dns/DNSNameService.java (revision 10452:8e501e6bbf1fda1bf9b19bc00fff02074162946f)
+++ dev/jdk/src/share/classes/sun/net/spi/nameservice/dns/DNSNameService.java (revision 10452+:8e501e6bbf1f+)
@@ -463,7 +463,7 @@
// ---------
- private static void appendIfLiteralAddress(String addr, StringBuffer sb) {
+ private static void appendIfLiteralAddress(String addr, StringBuilder sb) {
if (IPAddressUtil.isIPv4LiteralAddress(addr)) {
sb.append("dns://").append(addr).append(' ');
} else {
@@ -478,10 +478,8 @@
* corresponding to the supplied List of nameservers.
*/
private static String createProviderURL(List<String> nsList) {
- StringBuffer sb = new StringBuffer();
- for (String s: nsList) {
- appendIfLiteralAddress(s, sb);
- }
+ StringBuilder sb = new StringBuilder();
+ nsList.forEach(s -> appendIfLiteralAddress(s, sb));
return sb.toString();
}
@@ -491,7 +489,7 @@
* contained in the provided str.
*/
private static String createProviderURL(String str) {
- StringBuffer sb = new StringBuffer();
+ StringBuilder sb = new StringBuilder();
StringTokenizer st = new StringTokenizer(str, ",");
while (st.hasMoreTokens()) {
appendIfLiteralAddress(st.nextToken(), sb);
2. Also there are some minor issues with line width and readability:
--- dev/jdk/src/share/classes/java/security/cert/CertPath.java (revision 10452:8e501e6bbf1fda1bf9b19bc00fff02074162946f)
+++ dev/jdk/src/share/classes/java/security/cert/CertPath.java (revision 10452+:8e501e6bbf1f+)
@@ -224,16 +224,16 @@
Iterator<? extends Certificate> stringIterator =
getCertificates().iterator();
- sb.append('\n').append(type).append(" Cert Path: length = ").append(getCertificates().size()).append(".\n");
- sb.append("[\n");
+ sb.append('\n').append(type).append(" Cert Path: length = ")
+ .append(getCertificates().size()).append(".\n").append("[\n");
int i = 1;
while (stringIterator.hasNext()) {
- sb.append("==========================================" + "===============Certificate ")
- .append(i).append(" start.\n");
+ sb.append("==========================================")
+ .append("===============Certificate ").append(i).append(" start.\n");
Certificate stringCert = stringIterator.next();
- sb.append(stringCert.toString());
- sb.append("\n========================================" + "=================Certificate ")
- .append(i).append(" end.\n\n\n");
+ sb.append(stringCert.toString()).append('\n');
+ sb.append("========================================")
+ .append("=================Certificate ").append(i).append(" end.\n\n\n");
i++;
}
--- dev/jdk/src/share/classes/javax/swing/JColorChooser.java (revision 10452:8e501e6bbf1fda1bf9b19bc00fff02074162946f)
+++ dev/jdk/src/share/classes/javax/swing/JColorChooser.java (revision 10452+:8e501e6bbf1f+)
@@ -543,7 +543,7 @@
* @return a string representation of this <code>JColorChooser</code>
*/
protected String paramString() {
- StringBuilder chooserPanelsString = new StringBuilder("");
+ StringBuilder chooserPanelsString = new StringBuilder();
for (int i=0; i<chooserPanels.length; i++) {
chooserPanelsString.append('[').append(chooserPanels[i].toString()).append(']');
}
3. Btw, I wonder what "try ... catch" is doing here. Haven't we checked enum_.hasMoreElements() is true? (I guess it's a question to the security team).
--- dev/jdk/src/share/classes/java/security/PermissionCollection.java (revision 10452:8e501e6bbf1fda1bf9b19bc00fff02074162946f)
+++ dev/jdk/src/share/classes/java/security/PermissionCollection.java (revision 10452+:8e501e6bbf1f+)
@@ -181,13 +181,9 @@
StringBuilder sb = new StringBuilder();
sb.append(super.toString()).append(" (\n");
while (enum_.hasMoreElements()) {
- try {
- sb.append(' ');
- sb.append(enum_.nextElement().toString());
- sb.append('\n');
+ sb.append(' ');
+ sb.append(enum_.nextElement().toString());
+ sb.append('\n');
- } catch (NoSuchElementException e){
- // ignore
- }
}
sb.append(")\n");
return sb.toString();
4. This method looks dead (not used):
--- dev/jdk/src/share/classes/com/sun/tools/example/debug/gui/ContextManager.java (revision 10452:8e501e6bbf1fda1bf9b19bc00fff02074162946f)
+++ dev/jdk/src/share/classes/com/sun/tools/example/debug/gui/ContextManager.java (revision 10452+:8e501e6bbf1f+)
@@ -348,15 +348,4 @@
return javaArgs;
}
}
-
- private String appendPath(String path1, String path2) {
- if (path1 == null || path1.length() == 0) {
- return path2 == null ? "." : path2;
- } else if (path2 == null || path2.length() == 0) {
- return path1;
- } else {
- return path1 + File.pathSeparator + path2;
- }
- }
-
}
-Pavel
More information about the swing-dev
mailing list