4
\$\begingroup\$

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<ListGroupsObj>
    {

        @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<ListGroupsObj> listGroups = new ComboBox();

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

        listGroups.setEditable(true);

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

        // Display the selected Group
        listGroups.getSelectionModel().selectedItemProperty().addListener(new ChangeListener<ListGroupsObj>()
        {

            @Override
            public void changed(ObservableValue<? extends ListGroupsObj> arg0, ListGroupsObj arg1, ListGroupsObj arg2)
            {
                if (arg2 != null)
                {
                    System.out.println("Selected Group: " + arg2.getGroupId() + " - " + arg2.getGroupName());
                }
            }
        });

        final StackPane layout = new StackPane();
        layout.getChildren().add(listGroups);
        layout.setStyle("-fx-background-color: cornsilk; -fx-padding: 15;");
        stage.setScene(new Scene(layout));
        stage.show();
    }

    class GroupListCell extends ListCell<ListGroupsObj>
    {
        @Override
        protected void updateItem(ListGroupsObj item, boolean empty)
        {
            super.updateItem(item, empty);
            if (item != null)
            {
                setText(item.getGroupId() + " - " + item.getGroupName());
            }
        }
    }

    private List<ListGroupsObj> lisGroups;

    public static class ListGroupsObj
    {

        private int groupId;
        private String groupName;

        public static ListGroupsObj newInstance()
        {
            return new ListGroupsObj();
        }

        public ListGroupsObj()
        {
        }

        public ListGroupsObj groupId(int groupId)
        {
            this.groupId = groupId;
            return this;
        }

        public ListGroupsObj groupName(String groupName)
        {
            this.groupName = groupName;
            return this;
        }

        public int getGroupId()
        {
            return groupId;
        }

        public String getGroupName()
        {
            return groupName;
        }

        //        @Override
//        public String toString()
//        {
//            return serverName;
//        }
    }
}

How I can make the code more compact and easy for use?

\$\endgroup\$
  • \$\begingroup\$ What do you mean by "easy for use"? Are you planning on letting other people use this code as (part of) a library? \$\endgroup\$ – Simon Forsberg Dec 13 '13 at 20:32
3
\$\begingroup\$

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<ListGroupsObj> and Callback<ListView<ListGroupsObj>, ListCell<ListGroupsObj>> interfaces, this would let you use:

    listGroups.setCellFactory(this);
    

    and elsewhere in the Listobject class you put this method:

    @Override
    public ListCell<ListGroupsObj> call(ListView<ListGroupsObj> 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<ListGroupsObj> and Callback<ListView<ListGroupsObj>, ListCell<ListGroupsObj>> on an existing class instead of using anonymous classes:

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

    @Override
    public ListCell<ListGroupsObj> call(ListView<ListGroupsObj> p) {
        return new GroupListCell();
    }
    @Override
    public void changed(ObservableValue<? extends ListGroupsObj> 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<ListView<ListGroupsObj>, ListCell<ListGroupsObj>>, 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<ListGroupsObj>

        ...
    }
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.