Jeramiah's peer review

src/main/java/co/ppg2/controllers/GameController.java

@@ -20,7 +20,6 @@ public class GameController {
     private GameTimer gameTimer;
     private GameView gameView;
     private final ArrayList<Player> players;
-
     /**
      * Constructor for GameController.
      *
@@ -165,7 +164,12 @@ public Player getWinner(char token) {
      *
      * @param token The token of the winning player ('X' or 'O')
      */
-    public void updateLeaderboard(char token) {
+    /**
+     * Updates the player statistics after the game ends.
+     *
+     * @param token The token of the winning player ('X' or 'O')
+     */
+    public void updatePlayerStats(char token) {
         Player winner = getWinner(token);
         Player loser = (token == 'X') ? playerO : playerX;
 
@@ -177,6 +181,26 @@ public void updateLeaderboard(char token) {
             }
         }
 
+        PlayerDataController.savePlayers(players); // Persist updated player data
+    }
+
+    /**
+     * Displays the leaderboard popup with updated statistics.
+     */
+    public void displayLeaderboard() {
+        StringBuilder leaderboardDetails = new StringBuilder();
+
+        for (Player player : players) {
+            double avgTime = gameTimer.getAverageTimePerMove(player.getUsername());
+            leaderboardDetails.append(String.format("%s - Wins: %d, Losses: %d, Avg Time: %.2f seconds\n",
+                    player.getUsername(), player.getWins(), player.getLosses(), avgTime));
+        }
+
+        LeaderboardPopup.showLeaderboard(players); // Display the leaderboard UI
+    }
+// TODO What should be improved: The updateLeaderboard method mixes leaderboard update logic with UI generation, which violates the single-responsibility principle.
+        //TODO How it should be improved:  Split this method into two: one for updating player stats and another for generating the leaderboard display.
+
         PlayerDataController.savePlayers(players);
 
         // Build leaderboard details

src/main/java/co/ppg2/services/GameTimer.java

@@ -2,7 +2,8 @@
 
 import javafx.application.Platform;
 
-import java.util.HashMap;
+import java.util.concurrent.ConcurrentHashMap;
+
 import java.util.Map;
 
 /**
@@ -15,12 +16,12 @@ public class GameTimer implements Runnable {
     /**
      * Stores the total time spent by each player in milliseconds.
      */
-    private final Map<String, Long> playerTotalTime;
+    private final ConcurrentHashMap<String, Long> playerTotalTime;
 
     /**
      * Stores the number of moves made by each player.
      */
-    private final Map<String, Integer> playerMoves;
+    private final ConcurrentHashMap<String, Integer> playerMoves;
 
     /**
      * The name of the current player whose time is being tracked.
@@ -35,14 +36,17 @@ public class GameTimer implements Runnable {
     /**
      * Indicates whether the timer is currently running.
      */
-    private boolean running;
+    private volatile boolean running; // Use volatile for visibility across threads
+
 
     /**
      * Initializes a new {@link GameTimer} instance with no active players or timing data.
      */
+    //TODO What you should improve: playerTotalTime and playerMoves are modified in synchronized methods, but the run method isn't synchronized, which might lead to inconsistent states in multithreaded environments.
+    //TODO How to improve it: Wrap access to shared resources (playerTotalTime, playerMoves) in synchronized blocks or use a thread-safe structure like ConcurrentHashMap. And this will ensure thread saftey.
     public GameTimer() {
-        playerTotalTime = new HashMap<>();
-        playerMoves = new HashMap<>();
+        playerTotalTime = new ConcurrentHashMap<>();
+        playerMoves = new ConcurrentHashMap<>();
         running = false;
     }
 

src/main/java/co/ppg2/views/LabelWin.java

@@ -1,21 +1 @@
-package co.ppg2.views;
-
-/**
- * A custom label designed to display a win message with a specific style.
- * Extends {@link LabelBase} and overrides its default styling to indicate a win.
- */
-public class LabelWin extends LabelBase {
-    // Message to be displayed when a player wins
-    public String winMessage;
-
-    /**
-     * Constructs a LabelWin instance with the specified text.
-     * This label is styled with a light coral background to indicate a win.
-     *
-     * @param text the text to display on the win label.
-     */
-    public LabelWin(String text) {
-        super(text);
-        setStyle("-fx-background-color: lightcoral;");
-    }
-}
+//TODO labelWin is not used in the game so it should be removed