1
\$\begingroup\$

I've created a JavaFX app to the data about Vessels, Employees and Jobs. The current version works but it is quite slow which I need to fix. I understand the root of my problems but I do not know how to avoid it

I have several entities

class Vessel
{
    IntegerProperty id;
    StringProperty name;
    //and some other fields...
}

class Employee
{
    IntegerProperty id;
    StringProperty name;
    //and some other fields...
} 

class Job
{
    IntegerProperty id;
    StringProperty name;
    IntegerProperty employee;
    //and also a list of vessels assigned to it
    //and many more fields...
}

These entities correspond to the following tables (PostgreSQL)

CREATE TABLE vessels(id SERIAL PRIMARY KEY, "name" VARCHAR);
CREATE TABLE employees(id SERIAL PRIMARY KEY, "name" VARCHAR);
CREATE TABLE jobs(id SERIAL PRIMARY KEY, "name" VARCHAR);
CREATE TABLE jobs_vessels(id SERIAL PRIMARY KEY, job INT REFERENCES jobs(id), vessel INT REFERENCES vessels(id));

If I am dealing with Vessels, Employees or other simple entities (field are simple types), I execute

SELECT * FROM vessels ORDER BY "name";

followed by the standard JDBC code with Connection, PreparedStatement and ResultSet. Then I create ObservableList<Vessel> and populate it with the data from ResultSet. Next I create a TableView, initialize it's TableColumns (which are basically TextFieldTableCell) and finally display the data.

The problem arises when I want to display\modify a list of complex objects (fields are of type "class ..."). The first part would look the same... I execute

SELECT * FROM jobs ORDER BY "name";

and then I convert ResultSet resultSet to ObservableList<Job> jobs.

I also need to get vessels assigned to each job, so I do

protected Map<Job, List<int>> jobsVesselsMap;

//...

for (Job job : jobs)
{
    //1. get list of vessels (ids) assigned to the job via "SELECT * FROM jobs_vessels WHERE job = ?"
    //2. convert ResultSet to List<int>
    //3. store List<int> to "jobsVesselsMap".
}

Because I need to modify assigned vessels and an employee (to allow a user to select these via corresponding ComboBoxs), I need 2 more lists: list of vessels and list of employees. I execute

SELECT * FROM vessels ORDER BY "name";

and convert ResultSet into ObservableList<Vessel>. Then I execute

SELECT * FROM employees ORDER BY "name";

and convert ResultSet into ObservableList<Employee>.

Now comes the slow part. Because a complete "job" object should contain actual data about vessels and employees, I need to swap id's with actual objects. A complete job is represented by the object of type JobEx

class JobEx
{
    IntegerProperty id;
    StringProperty name;
    ObjectProperty<Employee> employee;
    ListProperty<Vessel> vessels;
    //and many more fields...
}

I convert ObservableList<Job> to ObservableList<JobEx> by calling toJobEx in a loop.

public static JobEx toJobEx(Job job,
                                    ObservableList<Vessel> vessels,
                                    ObservableList<Employee> employees,
                                    List<int> jobVesselsIds)
{
    JobEx jobEx = new JobEx();
    jobEx.setId(job.getId());
    jobEx.setName(job.getName());

    //employee
    Optional<Employee> employee = employees.stream().filter(e -> e.getId() == job.getEmployee()).findFirst();

    if (employee.isPresent())
    {
        jobEx.setVessel(employee.get());
    }

    //vessels
    ObservableList<Vessel> jobVessels = FXCollections.observableArrayList();

    for (int vesselId : jobVesselsIds)
    {
        Optional<Vessel> vessel = vessels.stream().filter(v -> v.getId() == vesselId).findFirst();

        if (vessel.isPresent())
        {
            jobEx.setVessel(vessel.get());
        }
    }

    jobEx.setVessels(jobVessels);

    return jobEx;
}

//...

ObservableList<JobEx> jobExs = FXCollections.observableArrayList();

for (Job job : jobs)
{
    jobExs.add(toJobEx(job));
}

At this step I have a complete list of jobs and I start to display it.

I create a TableView, create it's TableColumns, set corresponding editors for it's cells (TextFieldTableCell for StringPropertys, ComboBoxTableCell for ObjectProperty<Employees>, custom editor (multiple comboboxes + buttons) for `ListProperty'.

When I want save the data, I execute this process in reverse: ObservableList<JobEx> --> ObservableList<Job> --> save to multiple tables.

Now comboboxes inside the table display selected values correctly because fields of each JobEx instance and members of lists that were used to populate these comboboxes point to the same memory and same addresses which means that comboboxes select values automatically which is exactly what I want to achieve.

The positive side is that I can user JavaFX bindings and easily select\manipulate values via lists, comboboxes and similar UI controls. I don't know how to achieve the same by keeping ids (without "id to object, array of ids to lists of objects" approach). The drawback is that this Job to JobEx conversion take time (sometimes way to much) and in a real app there are a lot more "...Ex" classes that have objects instead of ids which result in a high amount of code and painful loading times.

Is there any way to avoid this. Ideally, I would like to completely remove this "to ...Ex conversion" part because is is slow and feels very incorrect.

\$\endgroup\$
1
\$\begingroup\$

One way to improve the problem section would be to look at it as an algorithm. For your business of I employees and J vessels, you have N jobs. You iterate over all jobs (N) and per job, you iterate over employees and vessels (worst case access time: N * I + N * J = N * (I + J) ). Since you iterate over the employee and vessel lists for every Job, your access times are going to eat your performance.


Solution

If you use a HashMap for vessels and employees, your access times for .contains and get/put reduce to most likely O(1) rather than O(J) and O(I) respectively. Lookup is easily mapped by making each employees ID its key within the map. Now you've reduced your algorithm from O(N * I + J) to O(N * 1 + 1) =~ O(N)


//employee
    Optional<Employee> employee = employees.stream().filter(e -> e.getId() == job.getEmployee()).findFirst();

    if (employee.isPresent())
    {
        jobEx.setVessel(employee.get());
    }

becomes

int employeeID = job.getEmployee();
if(employeeHashMap.containsKey(employeeID){
    jobEx.setVessel(employeeHashMap.get(employeeID))
}

and likewise,

//vessels
    ObservableList<Vessel> jobVessels = FXCollections.observableArrayList();

    for (int vesselId : jobVesselsIds)
    {
        Optional<Vessel> vessel = vessels.stream().filter(v -> v.getId() == vesselId).findFirst();

        if (vessel.isPresent())
        {
            jobEx.setVessel(vessel.get());
        }
    }

becomes

    ObservableList<Vessel> jobVessels = FXCollections.observableArrayList();

    for (int vesselId : jobVesselsIds)
    {
        if(vesselHashMap.containsKey(vesselId)
        {
            jobEx.setVessel(vesselHashMap.get(vesselID));
        }
    }

Lastly, some small bugs I noticed: Inside your employee loop you have this line

jobEx.setVessel(employee.get());

Do you mean to set the Vessel to your employee object?

Also, inside your vessel loop, you create a list

ObservableList<Vessel> jobVessels = FXCollections.observableArrayList();

never touch it,

 if (vessel.isPresent())
        {
            jobEx.setVessel(vessel.get());
        }

and then add the empty list

jobEx.setVessels(jobVessels);

Is this intentional? The internal part of this loop jobEx.setVessel(vessel.get()) would overide the JobEx vessel every time its called. If this method instead adds a new overall vessel, I would suggest renaming it addVessel() for accuracy

\$\endgroup\$
  • \$\begingroup\$ jobEx.setVessel(employee.get()) Yes, this is just a mistake I made when I created the question. \$\endgroup\$ – CorellianAle Apr 12 at 7:04

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.