patternjavaMinor
Console component in JavaFX
Viewed 0 times
componentconsolejavafx
Problem
I would like a review on my
```
public class Console extends BorderPane {
protected final TextArea textArea = new TextArea();
protected final TextField textField = new TextField();
protected final List history = new ArrayList<>();
protected int historyPointer = 0;
private Consumer 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(() -> {
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 history = new ArrayList<>();
protected int historyPointer = 0;
private Consumer 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(() -> {
Solution
As someone who's been working with your project and your Console Gui here's my 2 cents:
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:
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
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):
This also makes it easier to understand, where your constructor ends, without seeing the method signature that follows. While we're at it:
Why not put together what belongs together. Both of these are layouting commands, the other two are behavior commands. I'd go for:
Other than that, your code is very concise and understandable. I like your defensive programming style ;)
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 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 ;)
Code Snippets
protected final TextArea textArea = new TextArea();
protected final TextField textField = new TextField();protected final TextArea output = new TextArea();
protected final TextField input = new TextField();textField.addEventHandler(KeyEvent.KEY_RELEASED, keyEvent -> {
//long block of code...private final EventHandler<KeyEvent> keyReleasedHandler = new EventHandler<>() {
handle(final KeyEvent keyEvent){
//your code here
}
}
textField.setOnKeyReleased(keyReleasedHandler);setCenter(textArea);
//here comes the whole event handling
setBottom(textField);Context
StackExchange Code Review Q#52197, answer score: 8
Revisions (0)
No revisions yet.