Improve performance of StompCommand.getMessageType()

[dreis2211]

Closes SPR-14636

[kartoffelsup]

Couldn't this be done even better? Like:

enum StompCommand {
    CONNECT(SimpMessageType.CONNECT); // ,etc..

    private final SimpMessageType simpType;

    StompCommand(SimpMessageType simpType) {
      this.simpType = simpType;
    }
}

[dreis2211]

You're of course correct. Shows an even better uplift. Thanks

[kartoffelsup]

Instead of checking for null here, why not just pass SimpMessageType.OTHER instead of null to the constructors?

[kartoffelsup]

What do you think about this one?

public enum StompCommand {

  // client
  CONNECT(SimpMessageType.CONNECT, 0),
  STOMP(SimpMessageType.CONNECT, 0),
  DISCONNECT(SimpMessageType.DISCONNECT, 0),
  SUBSCRIBE(SimpMessageType.SUBSCRIBE, 3),
  UNSUBSCRIBE(SimpMessageType.UNSUBSCRIBE, 2),
  SEND(SimpMessageType.MESSAGE, 13),
  ACK(SimpMessageType.OTHER, 0),
  NACK(SimpMessageType.OTHER, 0),
  BEGIN(SimpMessageType.OTHER, 0),
  COMMIT(SimpMessageType.OTHER, 0),
  ABORT(SimpMessageType.OTHER, 0),

  // server
  CONNECTED(SimpMessageType.OTHER, 0),
  MESSAGE(SimpMessageType.MESSAGE, 15),
  RECEIPT(SimpMessageType.OTHER, 0),
  ERROR(SimpMessageType.OTHER, 12);

  private static final int DESTINATION_REQUIRED = 1;
  private static final int SUBSCRIPTION_ID_REQUIRED = 2;
  private static final int CONTENT_LENGTH_REQUIRED = 4;
  private static final int BODY_ALLOWED = 8;

  private final int flags;
  private final SimpMessageType simpMessageType;

  StompCommand(final SimpMessageType simpMessageType, final int flags) {
    this.flags = flags;
    this.simpMessageType = simpMessageType;
  }

  public SimpMessageType getMessageType() {
    return simpMessageType;
  }

  public boolean requiresDestination() {
    return (flags & DESTINATION_REQUIRED) != 0;
  }

  public boolean requiresSubscriptionId() {
    return (flags & SUBSCRIPTION_ID_REQUIRED) != 0;
  }

  public boolean requiresContentLength() {
    return (flags & CONTENT_LENGTH_REQUIRED) != 0;
  }

  public boolean isBodyAllowed() {
    return (flags & BODY_ALLOWED) != 0;
  }
}

public class StompCommandTest {

  private static final Collection<StompCommand> destinationRequired = asList(SEND, SUBSCRIBE, MESSAGE);
  private static final Collection<StompCommand> subscriptionIdRequired = asList(SUBSCRIBE, UNSUBSCRIBE, MESSAGE);
  private static final Collection<StompCommand> contentLengthRequired = asList(SEND, MESSAGE, ERROR);
  private static final Collection<StompCommand> bodyAllowed = asList(SEND, MESSAGE, ERROR);

  private static final Map<StompCommand, SimpMessageType> messageTypes = new EnumMap<>(StompCommand.class);

  static {
    messageTypes.put(StompCommand.CONNECT, SimpMessageType.CONNECT);
    messageTypes.put(StompCommand.STOMP, SimpMessageType.CONNECT);
    messageTypes.put(StompCommand.SEND, SimpMessageType.MESSAGE);
    messageTypes.put(StompCommand.MESSAGE, SimpMessageType.MESSAGE);
    messageTypes.put(StompCommand.SUBSCRIBE, SimpMessageType.SUBSCRIBE);
    messageTypes.put(StompCommand.UNSUBSCRIBE, SimpMessageType.UNSUBSCRIBE);
    messageTypes.put(StompCommand.DISCONNECT, SimpMessageType.DISCONNECT);
  }

  @Test
  public void getMessageType() throws Exception {
    for (final Map.Entry<StompCommand, SimpMessageType> stompToSimp : messageTypes.entrySet()) {
      assertEquals(stompToSimp.getKey().getMessageType(), stompToSimp.getValue());
    }
  }

  @Test
  public void requiresDestination() throws Exception {
    for (final StompCommand stompCommand : StompCommand.values()) {
      if (destinationRequired.contains(stompCommand)) {
        assertTrue(stompCommand.requiresDestination());
      } else {
        assertFalse(stompCommand.requiresDestination());
      }
    }
  }

  @Test
  public void requiresSubscriptionId() throws Exception {
    for (final StompCommand stompCommand : StompCommand.values()) {
      if (subscriptionIdRequired.contains(stompCommand)) {
        assertTrue(stompCommand.requiresSubscriptionId());
      } else {
        assertFalse(stompCommand.requiresSubscriptionId());
      }
    }
  }

  @Test
  public void requiresContentLength() throws Exception {
    for (final StompCommand stompCommand : StompCommand.values()) {
      if (contentLengthRequired.contains(stompCommand)) {
        assertTrue(stompCommand.requiresContentLength());
      } else {
        assertFalse(stompCommand.requiresContentLength());
      }
    }
  }

  @Test
  public void isBodyAllowed() throws Exception {
    for (final StompCommand stompCommand : StompCommand.values()) {
      if (bodyAllowed.contains(stompCommand)) {
        assertTrue(stompCommand.isBodyAllowed());
      } else {
        assertFalse(stompCommand.isBodyAllowed());
      }
    }
  }
}

[jhoeller]

Thanks for your proposals! I've implemented this along the lines of the flags suggestion, however with three individual boolean values instead... for the sake of readability since int constants can't be forward-referenced from enum value definitions... and we're not trying to optimize the memory layout to begin with.