Using Bukkit conversations for a coding console

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

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

StackExchange Code Review Q#41920, answer score: 4