I command thee: do SOMETHING

By Posted on

Problem

I am currenly contributing to a Chat-Bot to be used across the whole SE-Network’s chat which is implemented in Java 8.

This bot is supposed to have commands. These commands again are supposed to be as flexible as possible, and based on chat messages. To allow easy and extensible implementation of arbitrary commands I have created the following two classes:

  • CommandHandle: This class defines when a command is “triggered”, and specifies the code executed. It’s supposed to be as “generic” as possible while allowing clean and specialized code.
  • CommandHandleBuilder: This class is responsible for building instances of CommandHandles and defines a chainable API that’s supposed to be easy to use for anybody willing to create their own commands.

Currently the bot itself is a work in progress, but a working one can be found on github.

Anyways let’s get into code:

/**
 * Simple handle for a Command. Consists of a {@link Predicate} to match
 * messages (aka. invocations) against, a helpText,
 * an infoText and a {@link Consumer Consumer} for {@link ChatMessage ChatMessages}
 * 
 * @author vogel612<<a href="mailto:vogel612@gmx.de">vogel612@gmx.de</a>>
 */

public class CommandHandle {

    private final Predicate<String>     matchesSyntax;

    private final String                helpText;

    private final String                infoText;

    private final Consumer<ChatMessage> executor;

    public CommandHandle(
        Predicate<String> matchesSyntax, Consumer<ChatMessage> executor,
        String helpText, String infoText) {
        this.matchesSyntax = matchesSyntax;
        this.helpText = helpText;
        this.infoText = infoText;
        this.executor = executor;
    }

    public void execute(ChatMessage message) {
        executor.accept(message);
    }

    public boolean matchesSyntax(String commandCall) {
        return matchesSyntax.test(commandCall);
    }

    public String getHelpText() {
        return helpText;
    }

    public String getInfoText() {
        return infoText;
    }
}

/**
 * Command Builder for assembling commands. The command builder is not intended
 * to be threadsafe or reusable.
 * After building a command it should be disposed of, or undefined results may
 * occur
 * 
 * @author vogel612<<a href="mailto:vogel612@gmx.de">vogel612@gmx.de</a>>
 */
public final class CommandHandleBuilder {

    private static final Predicate<String> FALSE    = s -> false;

    private static final Logger            LOGGER   = Logger.getLogger(CommandHandleBuilder.class.getName());

    private Predicate<String>              matchesSyntax;

    private String                         helpText = "";

    private String                         infoText = "";

    private Consumer<ChatMessage>          executor;

    public CommandHandleBuilder() {
        matchesSyntax = FALSE;
    }

    /**
     * Build the command. Can be seen as ending the Builder's life span.
     * 
     * @return the resulting {@link CommandHandle} assembled from the actions
     *         performed on this instance.
     * @throws IllegalStateException
     *         if no syntax or execution were added.
     */
    public CommandHandle build() throws IllegalStateException {
        LOGGER.info("Building Command");
        if (FALSE == matchesSyntax || null == executor) { throw new IllegalStateException("Internal builder state does not allow building command yet, an execution and at least one syntax is required"); }

        return new CommandHandle(matchesSyntax, executor, helpText, infoText);
    }

    /**
     * Adds an additonal syntax to the command. Different Syntaxes are
     * shortcircuit-ORed together, as described by
     * {@link Predicate#or(Predicate) Predicate} This means if any of the given
     * syntaxes match the String later passed to the command it will return
     * true;
     * 
     * @param matcher
     *        A {@link Predicate Predicate<String>} describing the syntax
     * @return The Builder for chaining calls
     */
    public CommandHandleBuilder addSyntax(Predicate<String> matcher) {
        matchesSyntax = matchesSyntax.or(matcher);
        return this;
    }

    /**
     * Sets the command to be executed by the builder. Multiple calls to this
     * method overwrite each other, meaning only the latest given executor will
     * be in the built {@link CommandHandle}
     * 
     * @param executor
     *        the final version of the Command "handler"
     * @return The Builder for chaining calls
     */
    public CommandHandleBuilder setExecution(Consumer<ChatMessage> executor) {
        if (null != this.executor) {
            LOGGER.info("Overwriting existing executor");
        }

        this.executor = executor;
        return this;
    }

    /**
     * Sets the command's help text. The previously set helpText is overwritten,
     * only the latest given helpText will be added to the built {@link CommandHandle}
     * 
     * @param help
     *        The help text for the command
     * @return The Builder for chaining calls
     */
    public CommandHandleBuilder setHelpText(String help) {
        this.helpText = help;
        return this;
    }
    /**
    * Sets the command's info text. The previously set infoText is overwritten,
    * only the latest given infoText will be added to the built {@link CommandHandle}
    * 
    * @param info
    *        The info text for the command
    * @return The Builder for chaining calls
    */
    public CommandHandleBuilder setInfoText(String info) {
        this.infoText = info;
        return this;
    }
}

For demonstration, here’s a usage example:

CommandHandle shutdown = new CommandHandleBuilder().addSyntax(s -> {
          return s.trim().equals(TRIGGER + "shutdown");
      }).setExecution(message -> {
          chatInterface.broadcast("*~going down*");
          executor.shutdownNow();
          System.exit(0);
      }).setHelpText("Command shutdown: call using:'" + TRIGGER + "shutdown'. Terminates the bot")
      .setInfoText("Shuts down the executing JVM. The bot will no more respond")
      .build();

I am especially interested in possible improvements to the Builder and Naming.

The formatting is currently defined on a project-wide scale by the repository owner. While comments are appreciated, I doubt I will be able to change much there 😉

Solution

I recommend this implementation, see the explanation after:

class CommandHandle {

    private final Predicate<String> matchesSyntax;
    private final Consumer<ChatMessage> executor;
    private final String helpText = "";
    private final String infoText = "";

    public static class Builder {
        private final Predicate<String> matchesSyntax;
        private final Consumer<ChatMessage> executor;
        private String helpText;
        private String infoText;

        public Builder(Predicate<String> matchesSyntax, Consumer<ChatMessage> executor) {
            this.matchesSyntax = matchesSyntax;
            this.executor = executor;
        }

        public Builder setHelpText(String helpText) {
            this.helpText = helpText;
            return this;
        }

        public Builder setInfoText(String infoText) {
            this.infoText = infoText;
            return this;
        }
    }

    private CommandHandle(Builder builder) {
        this.matchesSyntax = builder.matchesSyntax;
        this.executor = builder.executor;
        this.helpText = builder.helpText;
        this.infoText = builder.infoText;
    }

    public static Builder builder(Predicate<String> matchesSyntax, Consumer<ChatMessage> executor) {
        return new Builder(matchesSyntax, executor);
    }

    public void execute(ChatMessage message) {
        executor.accept(message);
    }

    public boolean matchesSyntax(String commandCall) {
        return matchesSyntax.test(commandCall);
    }

    public String getHelpText() {
        return helpText;
    }

    public String getInfoText() {
        return infoText;
    }
}

Using it:

CommandHandle shutdown = CommandHandle.builder(s -> {
    return s.trim().equals(TRIGGER + "shutdown");
}, message -> {
    chatInterface.broadcast("*~going down*");
    executor.shutdownNow();
    System.exit(0);
}).setHelpText("Command shutdown: call using:'" + TRIGGER + "shutdown'. Terminates the bot")
.setInfoText("Shuts down the executing JVM. The bot will no more respond")
.build();

Comments / explanation / notes / improvements:

  • Common Builder pattern implementation:
    • The Builder is a static inner class
    • Allows simple and short Builder as name, instead of a longer qualified one like CommandHandleBuilder
    • The CommandHandle constructor is private
    • The CommandHandle constructor takes a Builder to construct itself
    • There is CommandHandle.builder helper for easier usage
  • The Builder has two required constructor arguments: the predicate and the consumer. This is much better than throwing a IllegalStateException. Because, does throwing IllegalStateException really help you that much? It will happen at runtime, and then what can you do about it? Nothing, really. By making it a constructor argument, the requirement of these two fields is enforced at compile time. You cannot accidentally not provide these arguments. This is safe and clear.
    • Btw, declaring a method with throws IllegalStateException also seems pointless, because it’s a runtime exception, so callers won’t be forced to handle it.
  • The most important fields (predicate and consumer) are higher and together, appearing before the less important help text and info text
  • I don’t see the point of logging when overwriting the predicate or the executor

If you are intent on using a builder, then I’d suggest either making it an inner static class to the main class or a different class in the same package. If the first solution, then the constructor for the built class can be made private; if the second, it can be made package local. First scenario:

public final class Foo
{
    // instance variables...

    public static Builder newBuilder()
    {
        return new Builder();
    }

    private Foo(args, here) { /* whatever */ }

    public static final class Builder
    {
        // etc etc

        public Foo build()
        {
            return new Foo(args, here);
        }
    }
}

Easy to adapt in the second scenario.

Second remark: I fail to see why FALSE is an illegal value at all for your predicate; as your code reads right now, I can very well input s -> false as an argument to the predicate and it will work. Why not just “let it be”? After all, it is the user’s fault if a correct predicate is not provided, isn’t it?

Third remark: calling a Consumer an executor? Uh… Why not just Consumer? Also, remind that any interface obeying the prototype of a @FunctionalInterface can be used, so you can define public interface MessageConsumer extends Consumer<ChatMessage> and use instances of this interface. By default, you could supply an empty one, or one that System.out.println()s, for instance

It horrifies me to say this, but I think you didn’t add enough padding.

private static final Predicate<String> FALSE    = s -> false;

private static final Logger            LOGGER   = Logger.getLogger(CommandHandleBuilder.class.getName());

private Predicate<String>              matchesSyntax;

private String                         helpText = "";

private String                         infoText = "";

private Consumer<ChatMessage>          executor;

matchesSyntax is longer than the other variable names, so you should add more padding to make the rest match.

This is also exactly why I hate this padding style. Try and see if you can configure either your editor or some beautifier to handle the padding automatically, because padding by hand eats valuable time.

if (FALSE == matchesSyntax || null == executor) { throw new IllegalStateException("Internal builder state does not allow building command yet, an execution and at least one syntax is required"); }
  1. Either write a statement on one line, and skip the braces, or write it over multiple lines and use the braces.
  2. “does not allow building command yet” – is this a TODO? What use is this to a maintenance programmer / API-using-developer? Point to ready alternatives, not future ones.

Leave a Reply

Your email address will not be published. Required fields are marked *