Console component in JavaFX

Question

I would like a review on my Console class I made in JavaFX 8. It features the following:

• A text field where you can enter input, the input gets copied into the text area.
• A text area where you can observe output and see the input.
• When you press enter, you send the message.
• When you press arrow up, you can go back in history.
• When you press arrow down, you can go forward in history.
• You can hook an event listener when a message gets sent.

---

public class Console extends BorderPane {
    protected final TextArea textArea = new TextArea();
    protected final TextField textField = new TextField();

    protected final List<String> history = new ArrayList<>();
    protected int historyPointer = 0;

    private Consumer<String> onMessageReceivedHandler;

    public Console() {
        textArea.setEditable(false);
        setCenter(textArea);

        textField.addEventHandler(KeyEvent.KEY_RELEASED, keyEvent -> {
            switch (keyEvent.getCode()) {
                case ENTER:
                    String text = textField.getText();
                    textArea.appendText(text + System.lineSeparator());
                    history.add(text);
                    historyPointer++;
                    if (onMessageReceivedHandler != null) {
                        onMessageReceivedHandler.accept(text);
                    }
                    textField.clear();
                    break;
                case UP:
                    if (historyPointer == 0) {
                        break;
                    }
                    historyPointer--;
                    runSafe(() -> {
                        textField.setText(history.get(historyPointer));
                        textField.selectAll();
                    });
                    break;
                case DOWN:
                    if (historyPointer == history.size() - 1) {
                        break;
                    }
                    historyPointer++;
                    runSafe(() -> {
                        textField.setText(history.get(historyPointer));
                        textField.selectAll();
                    });
                    break;
                default:
                    break;
            }
        });
        setBottom(textField);
    }

    @Override
    public void requestFocus() {
        super.requestFocus();
        textField.requestFocus();
    }

    public void setOnMessageReceivedHandler(final Consumer<String> onMessageReceivedHandler) {
        this.onMessageReceivedHandler = onMessageReceivedHandler;
    }

    public void clear() {
        runSafe(() -> textArea.clear());
    }

    public void print(final String text) {
        Objects.requireNonNull(text, "text");
        runSafe(() -> textArea.appendText(text));
    }

    public void println(final String text) {
        Objects.requireNonNull(text, "text");
        runSafe(() -> textArea.appendText(text + System.lineSeparator()));
    }

    public void println() {
        runSafe(() -> textArea.appendText(System.lineSeparator()));
    }
}

---

public final class GUIUtils {
    private GUIUtils() {
        throw new UnsupportedOperationException();
    }

    public static void runSafe(final Runnable runnable) {
        Objects.requireNonNull(runnable, "runnable");
        if (Platform.isFxApplicationThread()) {
            runnable.run();
        }
        else {
            Platform.runLater(runnable);
        }
    }
}

---

public class WordWrapConsole extends Console {
    public WordWrapConsole() {
        super();
        textArea.setWrapText(true);
    }
}

The calls that modify textArea and textField are wrapped in a GUIUtils.runSafe(), because modifications on JavaFX UI elements may only be done from the JavaFX thread and it is quite common that code from the public methods can run in different threads.

Answer 1

As someone who's been working with your project and your Console Gui here's my 2 cents:

protected final TextArea textArea = new TextArea();
protected final TextField textField = new TextField();

these could use a rename. The names do not contain any information and in all the unit tests you wrote, this was confusing me very much: Which is in, which is out?

I'd rather have something like:

protected final TextArea output = new TextArea();
protected final TextField input = new TextField();

---

textField.addEventHandler(KeyEvent.KEY_RELEASED, keyEvent -> {
    //long block of code...

I like very much that you are using a comprehensively wired lambda. I don't like that you are mixing abstraction levels here. on the one hand, you have that "I configure my console" code like textArea.setEditable(false), and then you have that switch (keyEvent.getCode())

Personally I'd have instead extracted the handler code to a separate handler (which needs a better name than what I am about to propose):

private final EventHandler<KeyEvent> keyReleasedHandler = new EventHandler<>() {
        handle(final KeyEvent keyEvent){
            //your code here
        }
    }
textField.setOnKeyReleased(keyReleasedHandler);

This also makes it easier to understand, where your constructor ends, without seeing the method signature that follows. While we're at it:

    setCenter(textArea);

    //here comes the whole event handling

    setBottom(textField);

Why not put together what belongs together. Both of these are layouting commands, the other two are behavior commands. I'd go for:

setCenter(output);
setBottom(input);

output.setEditable(false);
input.setOnKeyReleased(keyReleasedHandler);

---

Other than that, your code is very concise and understandable. I like your defensive programming style ;)

Answer 2

GUIUtils

This class should be GuiUtils, even if GUI is the "correct" abreviation you can still respect the naming convention. Just look at the HTTP classes from Apache and they are all respecting the convention (HttpClient,HttpGet, etc).

private GUIUtils() {
    throw new UnsupportedOperationException();
}

Do you really need to throw here ? You are protecting what by doing this ? I think you could just have the private constructor and leave it like this.

Object.requireNonNull

The second argument stand for a message to specify for the NullPointerException, I'm really not sure that specifying the name of the parameter is really a good message. In my opinion, it does not provide any insight that I will not have with the stacktrace, so you could add a relevant message or simply remove the argument.

WordWrapConsole

I'm not sure that this is a good idea to extends a class to add such tiny modification. I would imagine that if I want to add a new functionality to the Console I will need to extends it. So let's say I want to implement the clear functionality (cls), I will extends Console, but wait I want to word wrap I'll extends WordWrapConsole. Now I need a new functionality what will happen ? Well I'm "stuck" if I don't want word wrapping but I need the cls.

println

This is more an open question than a real comment about your code

public void println(final String text) {
    Objects.requireNonNull(text, "text");
    runSafe(() -> textArea.appendText(text + System.lineSeparator()));
}

public void println() {
    runSafe(() -> textArea.appendText(System.lineSeparator()));
}

The two implementations are so similar, I wonder if you could not "merge" them. println() could simply be println(""), but I don't find it elegant. The thing is you have two place that is writing almost the same way. Why not going further and say that println() is simply print(System.lineSeparator()) ?

Not printing null

It's good to be defensive, but just for your information you can do :

 String nullString = null;
 System.out.print(nullString);

This will print null. Is it really useful, do you want it, I guess not. Just wanted to say that sometimes null can still be valid (even if I don't like it) .

(You can't do System.out.print(null))