Using Bukkit conversations for a coding console

Question

I have a class that serves to carry data between my JavaScript evaluation class and the player, via the Bukkit Conversations API, available in full on GitHub (you will need Bukkit and Rhino as libraries (you can just comment out the JNBT code, it's not important), and a Craftbukkit server to run it). I was wondering in particular if I used inner classes correctly here, but also whether I should restructure my methods in some way.

public class CodePrompt extends StringPrompt {

    public final JavascriptRunner bridge;
    private boolean cmd = false;
    private String result = "";
    private String prevInput = "";
    public final Conversable coder;
    protected Conversation conversation;

    public Conversation getConversation() {
        return conversation;
    }

    public CodePrompt(final JavascriptRunner bridge, final Conversable coder) {
        this.bridge = bridge;
        this.coder = coder;
        //[API stuff]
        conversation = EvalJS.instance.convFactory.withFirstPrompt(this).buildConversation(coder);
    }

    @Override
    public String getPromptText(ConversationContext cc) {
        if (cmd) {
            return "";
        } else {
            return result;
        }
    }

    @Override
    public Prompt acceptInput(ConversationContext convContext, String input) {
        try {
            return eval(input);
        } catch (Resumable resumable) {
            return new InputPrompt(resumable);
        }
    }
    private Prompt eval(String input) throws Resumable {
        bridge.input(input); //on-before-evaluation function, by default just prints and logs the input, can be arbitrarily redefined
        if (input.length() == 0) {
            return this;
        }
        if (input.endsWith("\\")) {
            prevInput += input.substring(0, input.length() - 1);
            cmd = true;
            return this;
        }
        if (input.length() > 1 && input.endsWith("\\n")) {
            prevInput += input.substring(0, input.length() - 2) + "\n";
            cmd = true;
            return this;
        }
        input = prevInput + input;
        prevInput = "";
        if (input.startsWith("/")) {
            cmd = true;
            return doCommand(substitueInput(input), this);
        } else {
            cmd = false;
            try {
                final String output = bridge.eval(input);
                List<String> pages = paginate(output);
                if (pages.size() == 1) {
                    result = output;
                    return this;
                } else {
                    result = "";
                    return new Pager(pages);
                }
            } catch (ContinuationPending cc) {
                throw new EvaluationPending(cc);
            }
        }
    }

    private Prompt doCommand(String input, Prompt callback) {
        //[Run a built-in command. Due to how this works around API limitations, changing the return type to void and removing the callback argument would not work.]
        //Do-nothing implementation for testing:
        System.out.println("Running command " + input);
        return callback; //Real implementation does not actually return callback, but end behavior is similar to if it did
    }
    private String substitueInput(String input) throws SubstitutionPending {
        return substitueInput(input, new StringBuilder(), input.indexOf("$(") + 2, 0);
    }
    private String substitueInput(String input, StringBuilder substitutedInput, int from, int to) throws SubstitutionPending {
        if (from > 1) {
            substitutedInput.append(input.substring(to, from - 2));
            to = input.indexOf(")$", from) + 2;
            if (to < 2) {
                substitutedInput.append(input.substring(to, input.length()));
                return substitutedInput.toString();
            }
            try {
                substitutedInput.append(bridge.eval(input.substring(from, to - 2)));
            } catch (ContinuationPending cc) {
                throw new SubstitutionPending(input, substitutedInput, to, cc);
            }
        return substitueInput(input, substitutedInput, input.indexOf("$(", to) + 2, to);
        } else {
            substitutedInput.append(input.substring(to, input.length()));
            return substitutedInput.toString();
        }
    }

    public class InputPrompt extends StringPrompt {

        protected final Resumable resumable;
        public InputPrompt(Resumable resumable) {
            this.resumable = resumable;
        }

        @Override
        public String getPromptText(ConversationContext cc) {
            return "";
        }

        @Override
        public Prompt acceptInput(ConversationContext cc, String input) {
            try {
                return resumable.resume(input);
            } catch (Resumable subResumable) {
                return new InputPrompt(subResumable);
            }
        }
    }
    public abstract class Resumable extends Exception {

        public Resumable(Throwable cause) {
            super(cause);
        }
        public abstract Prompt resume(String newInput) throws Resumable;
    }
    public class SubstitutionPending extends Resumable {
        private final String input;
        private final StringBuilder substitutedInput;
        private final int to;
        @Override
        public Prompt resume(String newInput) throws SubstitutionPending {
            substitutedInput.append(bridge.resume(getCause(), newInput));
            return doCommand(substitueInput(input, substitutedInput, input.indexOf("$(", to) + 2, to), CodePrompt.this);
        }

        public SubstitutionPending(String input, StringBuilder substitutedInput, int to, ContinuationPending cause) {
            super(cause);
            this.input = input;
            this.substitutedInput = substitutedInput;
            this.to = to;
        }
        @Override
        public synchronized ContinuationPending getCause() {
            return (ContinuationPending)super.getCause();
        }
    }
    public class EvaluationPending extends Resumable {
        @Override
        public Prompt resume(String newInput) throws EvaluationPending {
            final String output = bridge.resume(getCause(), newInput);
            List<String> pages = paginate(output);
            if (pages.size() == 1) {
                result = output;
                return CodePrompt.this;
            } else {
                result = "";
                return new Pager(pages);
            }
        }

        public EvaluationPending(ContinuationPending cause) {
            super(cause);
        }
        @Override
        public synchronized ContinuationPending getCause() {
            return (ContinuationPending)super.getCause();
        }
    }

    public static final int LINECHARS = 44;//low estimate; font is not monospaced
    public static final int TEXTLINES = 20;
    public List<String> paginate(String result) {//Okay, maybe not the best name for a method.
        //[Split the input into strings that each fit in at most one page of the output text area]
        //Approximated for brevity:
        return Arrays.asList(result.split("\n")); //Real implementation has good reason for using List<String> instead of String[]
    }
    public class Pager extends ValidatingPrompt {

        private final List<String> pages;
        private int pos = 0;
        private boolean noDisplay = false;

        public Pager(List<String> pages) {
            this.pages = pages;
        }

        @Override
        public String getPromptText(ConversationContext cc) {
            //BUG: When Pager first becomes the active prompt, it doesn't display anything (works only once you provide 1 input). This method isn't called at all. I have no idea why.
            if (noDisplay) return "";
            return pages.get(pos) + "Page " + (pos + 1) + "/" + pages.size() + ". n:next p:prev §o#§r:page §o#§r q:quit";//§o = italics §r = end of italics
        }

        @Override
        protected boolean isInputValid(ConversationContext cc, String string) {
            if (string.startsWith("/") || string.equalsIgnoreCase("n") || string.equalsIgnoreCase("p") || string.equalsIgnoreCase("e") || string.equalsIgnoreCase("q")) {
                return true;
            }
            try {
                Integer.parseInt(string);
                return true;
            } catch (NumberFormatException ex) {
                return false;
            }
        }

        @Override
        protected Prompt acceptValidatedInput(ConversationContext cc, String string) {
            switch (string.toLowerCase()) {
                case "n":
                    if (pos + 1 >= pages.size()) {
                        cc.getForWhom().sendRawMessage("Invalid page!");
                        noDisplay = true;
                    } else {
                        pos++;
                        noDisplay = false;
                    }
                    return this;
                case "p":
                    if (pos - 1 <= 0) {
                        cc.getForWhom().sendRawMessage("Invalid page!");
                        noDisplay = true;
                    } else {
                        pos--;
                        noDisplay = false;
                    }
                    return this;
                case "e":
                case "q":
                    return CodePrompt.this;
                default:
                    if (string.startsWith("/")) {
                        noDisplay = true;
                        return doCommand(string, this);
                    } else {
                        int next = Integer.parseInt(string);
                        if (pos < 1 || pos > pages.size()) {
                            cc.getForWhom().sendRawMessage("Invalid page!");
                            noDisplay = true;
                        } else {
                            pos = next - 1;
                            noDisplay = false;
                        }
                        return this;
                    }
            }
        }
    }
}

Answer 1

Your code is actually pretty good overall. I have one very big question, though: why do you want all these to be inner classes? Normally, we use inner classes only for a very specific reason and only if the class in question is a critical component of the outer class and somehow inseparable. Usually, this is done with enums, for example:

public class Coin {

    public static enum Side {
        HEADS,
        TAILS;
    }

    // ...
}

For an example intrinsic to the Java API, you can look at Thread.State.

Here, it doesn't make any sense to have a Side without a Coin, and it makes the code more understandable to always reference a coin's side as something like Coin.Side.HEADS. We don't get much added benefit from extracting it into another class (i.e., CoinSide.HEADS).

Would you say this is the case for all of these inner classes? My personal opinion is "No." You're even creating whole hierarchies of classes in here, with your abstract class Resumable and then having other inner classes extend it. All of this is a bit complicated and all over the place for the function of a single class.

This is often quoted, but it's true: remember that a class, just like a method, should do one thing, and do it well. Consider extracting some of these classes into their own files, if you see fit and think it will clean up the code some.

---

Other than that, below are just some various tid bits. Take them or leave them.

public final JavascriptRunner bridge;
// ...
conversation = EvalJS.instance.convFactory.withFirstPrompt(this).buildConversation(coder);

In general, I'd prefer to have these kinds of objects (bridge, instance, convFactory) be accessed via getters and setters rather than being exposed as public. This is especially important for the one named instance, which I assume is supposed to be a singleton. A singleton's initialization should always be thread-safe, which means it needs its own synchronized accessor method, like so:

private Thingy instance = null;

public static synchronized Thingy getInstance() {
    if(instance == null) instance = new Thingy();
    return instance;
}

... or ...

private Thingy instance = null;
private Object INSTANCE_LOCK = new Object();

public static Thingy getInstance() {
    synchronized(INSTANCE_LOCK) {
        if(instance == null) {
            instance = new Thingy();
        }
    }
    return instance;
}

---

public String getPromptText(ConversationContext cc) {
    if (cmd) {
        return "";
    } else {
        return result;
    }
}

This could be rewritten in one line with the ternary operator: cmd ? "" : result;

---

private Prompt eval(String input) throws Resumable {

In general, you should try to have more descriptive method names. Following Java naming conventions, they should always be verbs or verb phrases. It just makes your code more readable and easier to follow. For example, this could be called evaluateInput() or something.

---

return (ContinuationPending)super.getCause();

I'm sure you're fine with this, but just be careful with unsafe casting! :) I like to check whether the cast is valid with either instanceof or, preferably, ContinuationPending.class.isInstance(super.getCause()).

---

public static final int LINECHARS = 44;//low estimate; font is not monospaced
public static final int TEXTLINES = 20;

Constants should almost always go at the top of your classes rather than being spread out through the file. Same with inner classes, as a matter of fact: group them all together either at the top or the bottom of the main class. In general, I tend to lay out my classes like this:

public class Whatever {

    //CONSTANTS

    //INNER ENUMS

    //CONSTRUCTORS

    //crtical methods
    //getters and setters
    //method overrides (e.g., `toString()`, `actionPerformed()`, ...)

    //STATIC METHODS

    //INNER CLASSES
}

That's just my particular style, but even if you don't like mine, there should be a consistent structure you use throughout your code.

---

if (string.startsWith("/") || string.equalsIgnoreCase("n") || string.equalsIgnoreCase("p") || string.equalsIgnoreCase("e") || string.equalsIgnoreCase("q"))

This is a bit of an ugly if statement. I'd clean it up with something like this:

private static final List<String> VALID_INPUT = Collections.unmodifiableList(Arrays.asList("n", "p", "e", "q"));

if(string.startsWith("/") || VALID_INPUT.contains(string.toLowerCase())) {
    //....

---

Finally...

return CodePrompt.this;

I'm a bit confused why you're accessing this statically here rather than just as this like you do throughout the rest of the code. Is there some obscure reason that I'm not aware of? If not, try to remain consistent.

Pretty high quality stuff overall!