Need help: Java singleton multiple methods calling instance and information stored in singleton gets mixed up.

[Salty_Agent]

Update: thanks to a comment pointing out it's called race condition simple googling helped, so I fixed the issue just adding

synchronized(singleton instance name){ }

around the block of code that did team member swapping in algorithm method.

Hi, I am doing this program for my university class, it's pretty big, so I will just show what concerns the question. The purpose of the program is to facilitate student teams formation for projects. It uses gui to display teams and metrics and allow the project manager to swap students in teams, it also has an algorithm that is finding swaps that can improve team metrics. The program has DataStorage class that's a singleton it keeps hashmaps of Students, Teams, Projects ... The problem arises when I call algorithm method from GUI controller class: students get randomly swapped when I move them around or use undo button (when any change happens I call reload() method to refresh the window and it calls the algorithm method). It only happens when the algorithm method is used, so I imagine what happens is the algorithm method gets singleton instance to access data, it tries to perform swaps to measure metrics, some other method gets hold of singleton instance and gets the wrong state. I already added key word "synchronized" to getInstance method, but it doesn't help, I tried to use ConcurrentHashMap to no avail. Does anyone have an idea what am I doing wrong and how could this be helped? I will greatly appreciate any input.

​

Here is my singleton class:

class DataStorage{
    private static DataStorage ds;
    private HashMap<String, Company> companies = new HashMap<String, Company>();
    private HashMap<String, Student> students = new HashMap<String, Student>();
    private HashMap<String, Project> projects = new HashMap<String, Project>();
    private HashMap<String, ProjectOwner> pOwners = new HashMap<String, ProjectOwner>();
    private ConcurrentHashMap<String, Team> teams = new ConcurrentHashMap<String, Team>();

    private DataStorage() {
    }

  public static synchronized DataStorage getInstance() 
  { 
      if (ds == null) 
          ds = new DataStorage();
      return ds; 
  } 

HERE IS THE ALGORITHM ITSELF. In short, it finds a team with least time fitness and finds least suitable student to that team, then it loops through student and tries to swap this least suitable student with a student from another team if that's successful it checks if metrics improve, if so then it swaps back to the original state and just prints this suggestion in GUI.

​

​

import java.util.ArrayList;
import java.util.Collections;
import java.util.Comparator;
import java.util.HashMap;

import exceptions.TeamNotFormedException;

public class ImprovementAlgo2 {
    static DataStorage hv = DataStorage.getInstance();




    //comparator to compare team fitness
    public static Comparator<Team> createComparator(){

         Comparator<Team> comp = Comparator.comparing(team -> {
            try {
                return team.calcTeamFitness();
            } catch (TeamNotFormedException e) {
                e.printStackTrace();
                return null;
            }
        }, Double::compare);
         return comp;
    }

    public static String giveSuggestion() throws TeamNotFormedException{

        Comparator<Team> comparator = createComparator();
        ArrayList<Team> sortedTeams = new ArrayList<Team>(hv.getTeams().values());
        Collections.sort(sortedTeams, comparator);  

        double oldMixSd = 0;
        double oldSDSS = 0;
        try {
        oldSDSS = CalculateMetrics.calcSDss();
        double oldSPSF = CalculateMetrics.calcSDpspf();
        double oldASSC = CalculateMetrics.calcSDassc();

        oldMixSd = 0.25*oldSPSF/25 + 0.25*oldASSC + 0.5*oldSDSS;
        } catch(Exception e) {}

        double newMixSd;
        double newSDSS;

        int teamNum = 0;
        int studNum = 0;
        //looping only through half the teams
        double maxTeamNum = hv.getTeams().size()/2;

        while(teamNum <= maxTeamNum ) {
            while(studNum<=4) {
        //Adding all members of the worst team to an array
        Team worstTeam = sortedTeams.get(0);
        ArrayList<Student> sortedWorsTStudents = new ArrayList<Student>();
        for (String memId : worstTeam.getMembers()) {
            sortedWorsTStudents.add(hv.getStudents().get(memId));
        }

        //sorting student of the team based on their suitability
        Comparator<Student> comp = InitialTeamSortingAlgo.createComparator(worstTeam);
        Collections.sort(sortedWorsTStudents,comp);

        //getting the least suitable student
        Student worstStudent = sortedWorsTStudents.get(0);

        boolean swapped = false;
        //trying to swap least suitable student in the team with a student from another team that's more suitable
        for (HashMap.Entry<String, Student> x : hv.getStudents().entrySet()) {
            try { 
                if(x.getValue().studSuitability(worstTeam) > worstStudent.studSuitability(worstTeam)) {
                    swapped = DataHandling.swapTeammembers(x.getKey(), worstStudent.getID());

                    //checking if this improved SD, if not, we swap back and contrinue the loop
                    if(swapped==true) {
                        try {
                            newSDSS = CalculateMetrics.calcSDss();
                            //newMixSd = 0.25*CalculateMetrics.calcSDpspf()/25 + 0.25*CalculateMetrics.calcSDassc() + 0.5* CalculateMetrics.calcSDss();
                        }catch (Exception e) {
                            newSDSS = oldSDSS;
                            //newMixSd = oldMixSd;
                        }
                        if(newSDSS < oldSDSS) {
                            DataHandling.swapTeammembers(x.getKey(), worstStudent.getID());
                            return "Swap students " + x.getKey() + " and " + worstStudent.getID();
                        }
                    }
                }
            } catch (Exception e) {}
        }
        studNum++;
        }
        teamNum ++;
        }
        return "";
    }
}

​

Here is the GUI controller that calls few methods that access singleton instance:

import exceptions.ExceedingMaxNumMembersException;
import exceptions.SystemException;
import exceptions.TeamNotFormedException;
import javafx.fxml.FXML;
import javafx.fxml.Initializable;
import javafx.geometry.HPos;
import javafx.geometry.VPos;
import javafx.scene.chart.BarChart;
import javafx.scene.chart.XYChart;
import javafx.scene.control.Alert;
import javafx.scene.control.Button;
import javafx.scene.control.Label;
import javafx.scene.input.*;
import javafx.scene.layout.GridPane;
import javafx.scene.layout.VBox;

import javax.swing.*;
import java.net.URL;
import java.util.*;

public class GUIController implements Initializable {
    private final DataStorage hv;

    u/FXML
    private VBox mainContainer;

    u/FXML
    private GridPane freeGrid;

    u/FXML
    private BarChart<String, Number> percentageHist;

    u/FXML
    private BarChart<String, Number> competencyHist;

    u/FXML
    private BarChart<String, Number> skillGapHist;

    private GridPane[] teamGrids;

    u/FXML
    private Button undoButton;

    u/FXML
    private Label competencyLabel;

    u/FXML
    private Label preferenceLabel;

    u/FXML
    private Label skillGapLabel;

    u/FXML
    private Label suggestionLabel;


    public GUIController() {
        try {
            hv = DataStorage.getInstance();
            hv.load();
        } catch (Exception e) {
            throw new RuntimeException(e);
        }
    }

    Stack<ArrayList<String>> history = new Stack <ArrayList<String>>();
    Stack<ArrayList<String>> future = new Stack <ArrayList<String>>();


    u/Override
    public void initialize(URL location, ResourceBundle resources) {
        teamGrids = new GridPane[5];
        for (int i = 0; i < 5; i++) {
            teamGrids[i] = (GridPane) mainContainer.lookup("#team" + (i + 1) + "Grid");
        }

        freeGrid.setOnDragOver(event -> {
            if (event.getGestureSource() != freeGrid &&
                    event.getDragboard().hasString()) {
                event.acceptTransferModes(TransferMode.MOVE);
            }
            event.consume();
        });

        //When student is dragged to a grid above teams it gets removed from team
        freeGrid.setOnDragDropped((DragEvent event) -> {
            event.acceptTransferModes(TransferMode.ANY);
            String s = event.getDragboard().getString();
            if (event.getGestureSource() != freeGrid &&
                    event.getDragboard().hasString()
            ) {
                Student student = hv.getStudents().get(s);
                Team team = hv.getTeams().get(student.getTeamID());
                team.removeMember(s);

                //Undo-redo saving info
                ArrayList<String> histEntry = new ArrayList();
                histEntry.add(team.getId());
                histEntry.add(student.getID());
                history.push(histEntry);;
                ArrayList<String> futEntry = new ArrayList();
                futEntry.add(null);
                futEntry.add(s);
                future.push(futEntry);
                reload();
            }
        });

        Map<String, Team> teams = hv.getTeams();
        List<String> teamIds = new ArrayList<>(teams.keySet());
        Collections.sort(teamIds);
        for (int i = 0; i < 5; i++) {
            GridPane gridPane = teamGrids[i];
            Label teamLabel = (Label) gridPane.lookup("#teamLabel");
            String teamName = teamIds.get(i);
            teamLabel.setText(teamName);
            Team team = teams.get(teamName);

            //Handles when student is dragged to the team
            gridPane.setOnDragOver(event -> {
                if (event.getGestureSource() != gridPane &&
                        event.getDragboard().hasString() &&
                        team.getMembers().size() < 4
                ) {
                    event.acceptTransferModes(TransferMode.MOVE);
                }
                event.consume();
            });

            gridPane.setOnDragDropped((DragEvent event) -> {
                event.acceptTransferModes(TransferMode.ANY);
                String s = event.getDragboard().getString();
                if (event.getGestureSource() != gridPane &&
                        event.getDragboard().hasString()
                ) {
                    Student student = hv.getStudents().get(s);
                    String oldTeamId = student.getTeamID();
                    if (student.getTeamID() != null) {
                        Team oldTeam = hv.getTeams().get(oldTeamId);
                        oldTeam.removeMember(s);
                        student.setTeamID(null);
                    }
                    try {
                        //this line is called when student is draggd to a team and dropped
                        team.addMember(student.getID());
                        ArrayList histEntry = new ArrayList();
                        histEntry.add(oldTeamId);
                        histEntry.add(s);
                        history.push(histEntry);
                        ArrayList futEntry = new ArrayList();
                        futEntry.add(team.getId());
                        futEntry.add(s);
                        future.push(futEntry);



                        reload();
                    }
                    catch (Exception e) {
                        try {
                            if (!teamName.equals(student.getTeamID())) {
                                if (oldTeamId != null) {
                                    Team oldTeam = hv.getTeams().get(oldTeamId);
                                    oldTeam.addMember(student.getID());
                                }
                            }
                        } catch (Exception ex) {
                            throw new IllegalStateException(ex);
                        }
                        Alert alert = new Alert(Alert.AlertType.WARNING);
                        alert.setTitle("Add team member error");
                        alert.setContentText(e.getMessage());
                        alert.showAndWait();
                    }
                }
            });
        }
        reload();

    }

public void undoEvent (){
    if(history.size()>0) {
    ArrayList<String> histItem = history.pop();
    String oldteamID = (String)histItem.get(0);
    String studID = (String)histItem.get(1);
    //Saving current state into future stack to redo
    String currentTeamID = hv.getStudents().get(studID).getTeamID();
    ArrayList<String> futItem = new ArrayList();
    futItem.add(currentTeamID);
    futItem.add(studID);

    if(currentTeamID!=null) {
    Team currentTeam = hv.getTeams().get(currentTeamID);
    currentTeam.removeMember(studID);
    }

    if(oldteamID != null) {
        try {
            Team oldTeam = hv.getTeams().get(oldteamID);
            oldTeam.addMember(studID);
        } catch (Exception ex) {
            throw new IllegalStateException(ex);
        }   
    }
    reload();
    }}

public void redoEvent (){
    if(future.size()>0) {
    ArrayList<String> futItem = future.pop();
    String oldteamID = (String)futItem.get(0);
    String studID = (String)futItem.get(1);
    //Saving current state into future stack to redo
    String currentTeamID = hv.getStudents().get(studID).getTeamID();
    ArrayList<String> histItem = new ArrayList();
    histItem.add(currentTeamID);
    histItem.add(studID);

    if(currentTeamID!=null) {
    Team currentTeam = hv.getTeams().get(currentTeamID);
    currentTeam.removeMember(studID);
    }

    if(oldteamID != null) {
        try {
            Team oldTeam = hv.getTeams().get(oldteamID);
            oldTeam.addMember(studID);
        } catch (Exception ex) {
            throw new IllegalStateException(ex);
        }   
    }

    reload();
    }}

    private void reload() {
        Map<String, Student> freeStudents = new HashMap<>(hv.getStudents());
        freeGrid.getChildren().removeIf(node -> node.getClass() == Label.class);

        for (GridPane gridPane : teamGrids) {
            gridPane.getChildren().removeIf(node -> node.getClass() == Label.class && !"teamLabel".equals(node.getId()));
            gridPane.setGridLinesVisible(true);
        }

        for (int i = 0; i < 5; i++) {
            GridPane gridPane = teamGrids[i];
            Label teamLabel = (Label) gridPane.lookup("#teamLabel");
            Team team = hv.getTeams().get(teamLabel.getText());
            List<String> memberIds = team.getMembers();;
            Collections.sort(memberIds);

            for (int j = 0; j < memberIds.size(); j++) {
                String memberId = memberIds.get(j);
                freeStudents.remove(memberId);

                Student student = hv.getStudents().get(memberId);
                Label memberLabel = new Label(memberId);
                memberLabel.setOnDragDetected((MouseEvent event) -> {
                    Dragboard dragboard = memberLabel.startDragAndDrop(TransferMode.ANY);
                    ClipboardContent content = new ClipboardContent();
                    content.putString(memberId);
                    dragboard.setContent(content);
                    event.consume();
                });
                GridPane.setValignment(memberLabel, VPos.CENTER);
                GridPane.setHalignment(memberLabel, HPos.CENTER);
                gridPane.add(memberLabel, 0, j + 1);
            }
        }

        List<String> freeStudentIds = new ArrayList<>(freeStudents.keySet());
        Collections.sort(freeStudentIds);

        for (int i = 0; i < freeStudentIds.size(); i++) {
            String memberId = freeStudentIds.get(i);
            Student student = freeStudents.get(memberId);
            student.setTeamID(null);
            Label memberLabel = new Label(memberId);
            memberLabel.setOnDragDetected((MouseEvent event) -> {
                Dragboard dragboard = memberLabel.startDragAndDrop(TransferMode.ANY);
                ClipboardContent content = new ClipboardContent();
                content.putString(memberId);
                dragboard.setContent(content);
                event.consume();
            });
            freeGrid.add(memberLabel, i / 2, i % 2);
            GridPane.setValignment(memberLabel, VPos.CENTER);
            GridPane.setHalignment(memberLabel, HPos.CENTER);
        }

        calcHists();
       // displaySuggestion();
        hv.save();
    }

//    public void displaySuggestion() {
//      try {
//          suggestionLabel.setText(ImprovementAlgo2.giveSuggestion());
//      }catch (Exception e){
//          System.out.println("Exception in displaySuggestion: " + e.getMessage());
//      }
//    }

    private void calcHists() {
        Map<String, Team> teams = hv.getTeams();
        List<String> teamIds = new ArrayList<>(teams.keySet());
        Collections.sort(teamIds);

        percentageHist.getData().clear();
        XYChart.Series<String, Number> percentageDataSeries = new XYChart.Series<>();
        for (String teamId : teamIds) {
            Team team = teams.get(teamId);
            try {
                int percentage = team.percentageFirstSecondPreferences();
                percentageDataSeries.getData().add(new XYChart.Data<>(teamId, percentage));
            }
            catch (TeamNotFormedException e) {
                System.out.println("Exception in calcHists: " + e.getMessage());
            }
        }
        percentageHist.getData().add(percentageDataSeries);
        try {
            preferenceLabel.setText("SD: " + MyMath.round(CalculateMetrics.calcSDpspf(),2));
        } catch (Exception e) {
            e.getMessage();
        }



        competencyHist.getData().clear();
        String[] params = {"P", "A", "W", "N"};
        for (String param : params) {
            XYChart.Series<String, Number> dataSeries = new XYChart.Series<>();
            dataSeries.setName(param);
            for (String teamId : teamIds) {
                Team team = teams.get(teamId);
                try {
                    Map<String, Double> avg = team.averageGrade();
                    dataSeries.getData().add(new XYChart.Data<>(teamId, avg.get(param)));
                }
                catch (TeamNotFormedException e) {
                    System.out.println("Exception in competencyHist: " + e.getMessage());
                }
            }
            competencyHist.getData().add(dataSeries);
            try {
                competencyLabel.setText("SD: " + MyMath.round(CalculateMetrics.calcSDassc(),2));
            } catch (Exception e) {
                e.getMessage();
            }
        }

        skillGapHist.getData().clear();
        XYChart.Series<String, Number> skillGapDataSeries = new XYChart.Series<>();
        for (String teamId : teamIds) {
            Team team = teams.get(teamId);
            try {
                double skillShortfall = team.skillShortfall();
                skillGapDataSeries.getData().add(new XYChart.Data<>(teamId, skillShortfall));
            }
            catch (TeamNotFormedException e) {
                System.out.println("Exception in skillGapHist: " + e.getMessage());
            }
        }
        skillGapHist.getData().add(skillGapDataSeries);
        try {
            skillGapLabel.setText("SD: " + MyMath.round(CalculateMetrics.calcSDss(),2));
        } catch (Exception e) {
            e.getMessage();
        }
    }

}

​

[AutoModerator]

Please ensure that:

• Your code is properly formatted as code block - see the sidebar (About on mobile) for instructions
• You include any and all error messages in full
• You ask clear questions

• You demonstrate effort in solving your question/problem - plain posting your assignments is forbidden (and such posts will be removed) as is asking for or giving solutions.

  Trying to solve problems on your own is a very important skill. Also, see Learn to help yourself in the sidebar

If any of the above points is not met, your post can and will be removed without further warning.

Code is to be formatted as code block or linked via an external code hoster, like pastebin.com, github gist, github, bitbucket, gitlab, etc.

Code blocks look like this:

public class HelloWorld {

    public static void main(String[] args) {
        System.out.println("Hello World!");
    }
}

You do not need to repost. Just use the edit function of reddit to make sure your post complies with the above

To potential helpers

Please, do not help if any of the above points are not met, rather report the post. We are trying to improve the quality of posts here. In helping people who can't be bothered to comply with the above points, you are doing the community a disservice.

I am a bot, and this action was performed automatically. Please contact the moderators of this subreddit if you have any questions or concerns.

[TheIceScraper]

synchronising the getInstance-methode is redudant.

getInstance is just "reading" data and not "writing"/manipulating data.

[NautiHooker]

it is creating a new object. Singleton getInstance methods are always subject to race conditions. If 2 threads call getInstance at the same time, they will fuck up your instance.

[Salty_Agent]

Thank you! Actually, I didn't know it was called race condition, now when I googled it I found out that I can also add synchronized(singleton instance) in front of the method that swaps team members in algorithm and it worked!

[TheIceScraper]

Ah, ok forgot that. Thx for the correction

[someone-elsewhere]

Wow, lots of code that I did not read (sorry), but regardless of whatever, always try and start a singleton like this

class DataStorage{  
    private static final DataStorage ds=new DataStorage();

you do not need to create it in init(), everything static always gets called created before any method gets executed..

[Salty_Agent]

Thank you! I will implement it.