This LoginPane is a Pain

Well, it really isn't a big pain: but I fear of security risks (if that is even possible).
Background:

I decided to (sort of) abandon my Sudoku project (because I accidentally deleted it from disk), and was given the idea of a JavaFX Helper Library. I started with the LoginPane, as I find I use login popups quite often.

The code is here:

```
import javafx.geometry.HPos;
import javafx.geometry.Insets;
import javafx.geometry.Pos;
import javafx.scene.Scene;
import javafx.scene.control.Button;
import javafx.scene.control.Label;
import javafx.scene.control.TextField;
import javafx.scene.layout.ColumnConstraints;
import javafx.scene.layout.GridPane;
import javafx.scene.layout.HBox;
import javafx.stage.Stage;

public class LoginPane {

public static final String DEFAULT_TITLE = "Login";

public static final String USERNAME_LABEL_DEFAULT_TEXT = "Username: ";
public static final String PASSWORD_LABEL_TEXT = "Password: ";

public static final String DEFAULT_LOGIN_TEXT = "Login";
public static final String CANCEL_TEXT = "Cancel";

public static final boolean DEFAULT_HAS_CANCEL = true;
public static final boolean DEFAULT_CAN_CLOSE = true;

private static final Insets MAIN_PANE_PADDING = new Insets(10);
private static final int MAIN_PANE_GAP = 10;
private static final int BUTTON_PREF_WIDTH = 60;
private static final int BUTTON_PANE_SPACING = 10;

private static final boolean IS_RESIZABLE = false;

private static final ColumnConstraints COL_1_CONSTRAINS = new ColumnConstraints(
70);
private static final ColumnConstraints COL_2_CONSTRAINS = new ColumnConstraints(
200);

private static String username = null;
private static char[] password = null;

public static UserInfo showLoginPane() {
return showLoginPane(DEFAULT_TITLE);
}

public static UserInfo showLoginPane(String title) {
return showLoginPane(title, USERNAME_LABEL_DEFAULT_TEXT);

Constants

All the public statics should be private. If clients really care, you can document the default label, but then you can't change the label without it being an API spec change. I wouldn't expect most clients need to know the default. Either they care to specify or they don't.

For the privates, my personal preference would be to inline all the ones that will only ever be used in one place, but I can see the argument for extracting them.

Design

I would suggest making a LoginPane an instantiable class, rather than just a helper method. Conceptually, a LoginPane is a thing, and so representing instances as objects would seem to be reasonable. It would have a constructor and a public UserInfo show() method, which would make it reusable.

You should also consider using the Builder pattern to avoid the telescoping static method problem that you have. It would allow clients to specify only the values they care about. Documentation can indicate defaults - a good idea for the booleans, at least, though per above the labels are debatable.

Naming

For consistency, canCancel would be better than hasCancel. The documentation can indicate that the cancel option is only made available if canCancel is true. Non-abbreviated names are generally better - COLUMN_1_CONSTRAINTS > COL_1_CONSTRAINTS.

Correctness

The passwordLabel and cancelButton don't use the non-default label values, even if they're provided as method arguments.

I'm not sure how much making the password a char[] actually helps you, because it's already in memory as a String from getText(). I'm not a JavaFX expert, so I'm not sure if there's a more correct way to do it, but a quick search implies there is not.

You have a bug at the intersection of canClose and hasCancel. If hasCancel is true and canClose is false, you render a cancel button which does nothing.

You also don't differentiate at all between somebody hitting [Ok] without entering any values and somebody hitting [Cancel]. It's unclear if that's relevant to clients, but probably it is.

Here's something I slapped together that addresses many of the issues I discussed.

```
public final class LoginPane {

private static final Insets MAIN_PANE_PADDING = new Insets(10);
private static final int MAIN_PANE_GAP = 10;
private static final int BUTTON_PREFERRED_WIDTH = 60;
private static final int BUTTON_PANE_SPACING = 10;

private static final boolean IS_RESIZABLE = false;

private static final ColumnConstraints COLUMN_1_CONSTRAINS = new ColumnConstraints(70);
private static final ColumnConstraints COLUMN_2_CONSTRAINS = new ColumnConstraints(200);

private final String username = "";
private final char[] password = new char[0];
private final Stage stage = new Stage();

private LoginPane(final Builder builder) {
final GridPane mainPane = new GridPane();
mainPane.setPadding(MAIN_PANE_PADDING);
mainPane.setHgap(MAIN_PANE_GAP);
mainPane.setVgap(MAIN_PANE_GAP);
mainPane.getColumnConstraints().addAll(COLUMN_1_CONSTRAINS, COLUMN_2_CONSTRAINS);

final Label userLabel = new Label(builder.usernameLabel);
GridPane.setHalignment(userLabel, HPos.RIGHT);

final TextField usernameField = new TextField();
GridPane.setHalignment(usernameField, HPos.LEFT);

mainPane.addRow(0, userLabel, usernameField);

final Label passwordLabel = new Label(builder.passwordLabel);
GridPane.setHalignment(passwordLabel, HPos.RIGHT);

final PasswordField passwordField = new PasswordField();
GridPane.setHalignment(passwordField, HPos.LEFT);

mainPane.addRow(1, passwordLabel, passwordField);

final HBox buttonPane = new HBox(BUTTON_PANE_SPACING);

final Button login = new Button(builder.loginText);
login.setPrefWidth(BUTTON_PREFERRED_WIDTH);
buttonPane.getChildren().add(login);
login.setOnAction(e -> {
username = usernameField.getText();
password = passwordField.getText().toCharArray();
});

if (builder.canCancel) {
final Button cancel = new Button(builder.cancelText);
cancel.setPrefWidth(BUTTON_PREFERRED_WIDTH);
buttonPane.getChildren().add(cancel);
cancel.setOnAction(e -> stage.close());
}

buttonPane.setAlignment(Pos.CENTER_RIGHT);

mainPane.add(buttonPane, 1, 2);
GridPane.setHalignment(buttonPane, HPos.RIGHT);
final Scene scene = new Scene(mainPane);

this.stage.setTitle(builder.title);
this.stage.setScene(scene);
this.stage.setResizable(IS_RESIZABLE);
this.stage.setOnCloseRequest(e -> {
if (builder.canCancel || builder.canClose) {
stage.close();
} else {
e.consume();
}
});

}

public UserInfo showAndWait() {
this.stage.showAndWait();
re

StackExchange Code Review Q#107988, answer score: 2