Listing groups by ID and name

I have this JavaFX code which is used for listing groups by ID and name:

```
import java.util.List;
import javafx.application.Application;
import javafx.beans.value.ChangeListener;
import javafx.beans.value.ObservableValue;
import javafx.scene.Scene;
import javafx.scene.control.ComboBox;
import javafx.scene.control.ListCell;
import javafx.scene.control.ListView;
import javafx.scene.layout.StackPane;
import javafx.stage.Stage;
import javafx.util.Callback;
import javafx.util.StringConverter;

public class Listobject extends Application
{

public class GroupConverter extends StringConverter
{

@Override
public String toString(ListGroupsObj obj)
{
return obj.getGroupId() + " - " + obj.getGroupName();
}

@Override
public ListGroupsObj fromString(String obj)
{

return ListGroupsObj.newInstance().groupName(obj);
}

}

public static void main(String[] args)
{
launch(args);
}

@Override
public void start(Stage stage)
{

// Insert Some data
ListGroupsObj ob = ListGroupsObj.newInstance().groupId(12).groupName("Group12");
ListGroupsObj osb = ListGroupsObj.newInstance().groupId(13).groupName("Group13");
ListGroupsObj oa = ListGroupsObj.newInstance().groupId(14).groupName("Group14");
ListGroupsObj oz = ListGroupsObj.newInstance().groupId(15).groupName("Group15");

final ComboBox listGroups = new ComboBox();

listGroups.setConverter(new GroupConverter());
listGroups.setButtonCell(new GroupListCell());
listGroups.setCellFactory(new Callback, ListCell>()
{
@Override
public ListCell call(ListView p)
{
return new GroupListCell();
}
});

listGroups.setEditable(true);

listGroups.getItems().addAll(ob, osb, oa, oz);
listGroups.setValue(ob);

// Display the selected Group

I have to admit that I don't know much about JavaFX, but I do know Java so I can help you with the things I can come up with:

If by "more compact" you mean "less number of lines" there are several things that you can do:

• Use Java coding conventions and don't put { on a new line. That alone will save several lines.

• You have unnecessary empty lines on some places, for example in the beginning of methods.

• Remove commented code, such as your toString method (Also, including commented code in your question here will only make us confused - Do you want your commented code to be reviewed as well? - Rhetorical question, no need to answer that)

• Remove public empty constructors, those are added automatically by Java

-
Instead of using anonymous inner classes, you can let your Listobject class implement the ChangeListener and Callback, ListCell> interfaces, this would let you use:

listGroups.setCellFactory(this);

and elsewhere in the Listobject class you put this method:

@Override
public ListCell call(ListView p) {
    return new GroupListCell();
}

if you use this approach, you might want to extract an interface for your Listobject class, since those methods will be a part of it's public API (although there's not much harm in letting them be a part of the class' public API, it might confuse users of your class).

A couple of other notes:

• Are you sure you want GroupConverter to be a public static class? You might want to set it to private.

• GroupListCell could be, and probably should be, a private static class

• Not really sure why you are using a factory method for the ListGroupsObj class. Instead you could pass groupId and groupName to the constructor and make the two fields final and you will have yourself a nice immutable class. Then you can remove your "setters" for this class.

Edit: Adding a sample of code to show how to implement ChangeListener and Callback, ListCell> on an existing class instead of using anonymous classes:

public class Listobject extends Application implements Callback, ListCell>, ChangeListener {

    @Override
    public ListCell call(ListView p) {
        return new GroupListCell();
    }
    @Override
    public void changed(ObservableValue arg0, ListGroupsObj arg1, ListGroupsObj arg2) {
        if (arg2 != null) {
            System.out.println("Selected Group: " + arg2.getGroupId() + " - " + arg2.getGroupName());
        }
    }

    @Override
    public void start(Stage stage)
    {
        ...

        listGroups.setConverter(new GroupConverter());
        listGroups.setButtonCell(new GroupListCell());
        listGroups.setCellFactory(this); // "this" means "the current Listobject"
        // the current Listobject is also a Callback, ListCell>, which is what the `setCellFactory` method wants

        listGroups.setEditable(true);

        listGroups.getItems().addAll(ob, osb, oa, oz);
        listGroups.setValue(ob);

        // Display the selected Group
        listGroups.getSelectionModel().selectedItemProperty().addListener(this); // "this" is a Listobject, which is also a ChangeListener

        ...
    }
}

StackExchange Code Review Q#37320, answer score: 3