[GitHub] [activemq-artemis] gemmellr commented on a change in pull request #3455: NO-JIRA Avoid creation of unnecessary StringBuilder instance in Netty…

[GitBox <gi...@apache.org>]

gemmellr commented on a change in pull request #3455:
URL: https://github.com/apache/activemq-artemis/pull/3455#discussion_r576860189



##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/remoting/impl/netty/NettyAcceptor.java
##########
@@ -863,15 +863,16 @@ public NettyAcceptor setShutdownTimeout(int shutdownTimeout) {
    }
 
    private static String getProtocols(Map<String, ProtocolManager> protocolManager) {
+      if (protocolManager == null || protocolManager.isEmpty())
+         return "";
+
       StringBuilder sb = new StringBuilder();
-      if (protocolManager != null) {
-         Set<String> strings = protocolManager.keySet();
-         for (String string : strings) {
-            if (sb.length() > 0) {
-               sb.append(",");
-            }
-            sb.append(string);
+      Set<String> strings = protocolManager.keySet();
+      for (String string : strings) {

Review comment:
       Again if the aim is readability, using protocolManager(s).forEach(..) would seem nicer than seperate .keySet() and for-each loop, and perhaps also more efficient depending on impl.

##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/remoting/impl/netty/NettyAcceptor.java
##########
@@ -863,15 +863,16 @@ public NettyAcceptor setShutdownTimeout(int shutdownTimeout) {
    }
 
    private static String getProtocols(Map<String, ProtocolManager> protocolManager) {
+      if (protocolManager == null || protocolManager.isEmpty())

Review comment:
       If we are aiding readability here, what about making the variable name "protocolManagers" since it is a map of them, not a single instance.
   
   If statements are nicer with braces I find, less screwups later.
   
   I have no idea where this is called from but I'm also initially surprised it doesnt just throw if the map is null/empty. When is it expected/valid to have no protocols?




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org