Apply code review

5 files changed, 106 insertions, 114 deletions

diff --git a/src/main/java/de/hysky/skyblocker/config/configs/MiningConfig.java b/src/main/java/de/hysky/skyblocker/config/configs/MiningConfig.java
index 1347f158..dcf70f24 100644
--- a/src/main/java/de/hysky/skyblocker/config/configs/MiningConfig.java
+++ b/src/main/java/de/hysky/skyblocker/config/configs/MiningConfig.java
@@ -108,10 +108,10 @@ public class MiningConfig {
         public CommissionWaypointMode mode = CommissionWaypointMode.BOTH;
 
         @SerialEntry
-        public boolean useColor = true;
+        public float textScale = 1;
 
         @SerialEntry
-        public float textScale = 1;
+        public boolean useColor = true;
 
         @SerialEntry
         public boolean showBaseCamp = false;
diff --git a/src/main/java/de/hysky/skyblocker/skyblock/dwarven/CrystalsLocationsManager.java b/src/main/java/de/hysky/skyblocker/skyblock/dwarven/CrystalsLocationsManager.java
index ac8e413b..8ccafed2 100644
--- a/src/main/java/de/hysky/skyblocker/skyblock/dwarven/CrystalsLocationsManager.java
+++ b/src/main/java/de/hysky/skyblocker/skyblock/dwarven/CrystalsLocationsManager.java
@@ -187,14 +187,8 @@ public class CrystalsLocationsManager {
     }
 
     protected static Text getSetLocationMessage(String location, BlockPos blockPos) {
-        MutableText text = Constants.PREFIX.get();
-        text.append(Text.translatable("skyblocker.config.mining.crystalsWaypoints.addedWaypoint"));
         int locationColor = WAYPOINT_LOCATIONS.get(location).getColor();
-        text.append(Text.literal(location).withColor(locationColor));
-        text.append(Text.literal(" ").append(Text.translatable("skyblocker.config.mining.crystalsWaypoints.addedWaypoint.at")));
-        text.append(Text.literal(" : " + blockPos.getX() + " " + blockPos.getY() + " " + blockPos.getZ() + "."));
-
-        return text;
+        return Constants.PREFIX.get().append(Text.translatable("skyblocker.config.mining.crystalsWaypoints.addedWaypoint", Text.literal(location).withColor(locationColor), blockPos.getX(), blockPos.getY(), blockPos.getZ()));
     }
 
     /**
@@ -205,18 +199,14 @@ public class CrystalsLocationsManager {
      * @return text for a message to send to the player
      */
     private static Text getLocationMenu(String location, boolean excludeUnknown) {
-        MutableText text = Constants.PREFIX.get();
 
         //if the user has all available waypoints active warn them instead of an empty list (excused unknown from check when disabled)
         if (activeWaypoints.size() == WAYPOINT_LOCATIONS.size() || (excludeUnknown && WAYPOINT_LOCATIONS.size() - activeWaypoints.size() == 1 && !activeWaypoints.containsKey(MiningLocationLabel.CrystalHollowsLocationsCategory.UNKNOWN.getName()))) {
-            return text.append(Text.translatable("skyblocker.config.mining.crystalsWaypoints.allActive").formatted(Formatting.RED));
+            return Constants.PREFIX.get().append(Text.translatable("skyblocker.config.mining.crystalsWaypoints.allActive").formatted(Formatting.RED));
         }
 
         //add starting message
-        text.append(Text.translatable("skyblocker.config.mining.crystalsWaypoints.MarkLocation.start")
-                .append(Text.literal(" ("+location+") ").formatted(Formatting.GRAY))
-                .append(Text.translatable("skyblocker.config.mining.crystalsWaypoints.MarkLocation.end"))
-        );
+        MutableText text = Text.empty();
 
         //add possible locations to the message
         for (String waypointLocation : WAYPOINT_LOCATIONS.keySet()) {
@@ -231,10 +221,9 @@ public class CrystalsLocationsManager {
             );
         }
 
-        return text;
+        return Constants.PREFIX.get().append(Text.translatable("skyblocker.config.mining.crystalsWaypoints.markLocation", location, text));
     }
 
-
     /**
      * Creates a formated text with a list of found places to remove / share a waypoint for
      *
@@ -250,20 +239,20 @@ public class CrystalsLocationsManager {
         }
 
         //depending on the action load the correct prefix and hover message
-        String hoverMessage;
+        MutableText hoverMessage;
         if (action.equals("remove")) {
             text.append(Text.translatable("skyblocker.config.mining.crystalsWaypoints.getLocationHover.remove").append(Text.literal(": ")));
-            hoverMessage = "skyblocker.config.mining.crystalsWaypoints.getLocationHover.remove";
+            hoverMessage = Text.translatable("skyblocker.config.mining.crystalsWaypoints.getLocationHover.remove");
         } else {
             text.append(Text.translatable("skyblocker.config.mining.crystalsWaypoints.getLocationHover.share").append(Text.literal(": ")));
-            hoverMessage = "skyblocker.config.mining.crystalsWaypoints.getLocationHover.share";
+            hoverMessage = Text.translatable("skyblocker.config.mining.crystalsWaypoints.getLocationHover.share");
         }
-        
+
         for (String waypointLocation : activeWaypoints.keySet()) {
             int locationColor = WAYPOINT_LOCATIONS.get(waypointLocation).getColor();
             text.append(Text.literal("[" + waypointLocation + "]").withColor(locationColor).styled(style -> style
                     .withClickEvent(new ClickEvent(ClickEvent.Action.RUN_COMMAND, "/skyblocker crystalWaypoints " + action + " " + waypointLocation))
-                    .withHoverEvent(new HoverEvent(HoverEvent.Action.SHOW_TEXT, Text.translatable(hoverMessage).withColor(locationColor))))
+                    .withHoverEvent(new HoverEvent(HoverEvent.Action.SHOW_TEXT, hoverMessage.withColor(locationColor))))
             );
         }
 
diff --git a/src/main/java/de/hysky/skyblocker/skyblock/dwarven/MiningLocationLabel.java b/src/main/java/de/hysky/skyblocker/skyblock/dwarven/MiningLocationLabel.java
index 2c3409c5..3817f6c7 100644
--- a/src/main/java/de/hysky/skyblocker/skyblock/dwarven/MiningLocationLabel.java
+++ b/src/main/java/de/hysky/skyblocker/skyblock/dwarven/MiningLocationLabel.java
@@ -14,6 +14,7 @@ import net.minecraft.util.math.Vec3d;
 
 import java.awt.*;
 
+// TODO: Clean up into the waypoint system with a new `DistancedWaypoint` that extends `NamedWaypoint` for this and secret waypoints.
 public record MiningLocationLabel(Category category, Vec3d centerPos) implements Renderable {
     public MiningLocationLabel(Category category, BlockPos pos) {
         this(category, pos.toCenterPos());
diff --git a/src/main/java/de/hysky/skyblocker/skyblock/dwarven/WishingCompassSolver.java b/src/main/java/de/hysky/skyblocker/skyblock/dwarven/WishingCompassSolver.java
index 7e7386f0..d513a38b 100644
--- a/src/main/java/de/hysky/skyblocker/skyblock/dwarven/WishingCompassSolver.java
+++ b/src/main/java/de/hysky/skyblocker/skyblock/dwarven/WishingCompassSolver.java
@@ -9,13 +9,15 @@ import net.fabricmc.fabric.api.client.networking.v1.ClientPlayConnectionEvents;
 import net.fabricmc.fabric.api.event.player.UseBlockCallback;
 import net.fabricmc.fabric.api.event.player.UseItemCallback;
 import net.minecraft.client.MinecraftClient;
-import net.minecraft.client.network.PlayerListEntry;
 import net.minecraft.entity.player.PlayerEntity;
 import net.minecraft.item.ItemStack;
 import net.minecraft.network.packet.s2c.play.ParticleS2CPacket;
 import net.minecraft.particle.ParticleTypes;
 import net.minecraft.text.Text;
-import net.minecraft.util.*;
+import net.minecraft.util.ActionResult;
+import net.minecraft.util.Formatting;
+import net.minecraft.util.Hand;
+import net.minecraft.util.TypedActionResult;
 import net.minecraft.util.hit.BlockHitResult;
 import net.minecraft.util.math.BlockPos;
 import net.minecraft.util.math.Box;
@@ -24,45 +26,27 @@ import net.minecraft.world.World;
 import org.apache.commons.math3.geometry.euclidean.threed.Line;
 import org.apache.commons.math3.geometry.euclidean.threed.Vector3D;
 
-import java.util.HashMap;
 import java.util.Map;
 import java.util.Objects;
 import java.util.stream.Stream;
 
 public class WishingCompassSolver {
     private static final MinecraftClient CLIENT = MinecraftClient.getInstance();
-
-    enum SolverStates {
-        NOT_STARTED,
-        PROCESSING_FIRST_USE,
-        WAITING_FOR_SECOND,
-        PROCESSING_SECOND_USE,
-    }
-
-    enum ZONE {
-        CRYSTAL_NUCLEUS,
-        JUNGLE,
-        MITHRIL_DEPOSITS,
-        GOBLIN_HOLDOUT,
-        PRECURSOR_REMNANTS,
-        MAGMA_FIELDS,
-    }
-
-    private static final HashMap<ZONE, Box> ZONE_BOUNDING_BOXES = Util.make(new HashMap<>(), map -> {
-        map.put(ZONE.CRYSTAL_NUCLEUS, new Box(462, 63, 461, 564, 181, 565));
-        map.put(ZONE.JUNGLE, new Box(201, 63, 201, 513, 189, 513));
-        map.put(ZONE.MITHRIL_DEPOSITS, new Box(512, 63, 201, 824, 189, 513));
-        map.put(ZONE.GOBLIN_HOLDOUT, new Box(201, 63, 512, 513, 189, 824));
-        map.put(ZONE.PRECURSOR_REMNANTS, new Box(512, 63, 512, 824, 189, 824));
-        map.put(ZONE.MAGMA_FIELDS, new Box(201, 30, 201, 824, 64, 824));
-    });
+    private static final Map<Zone, Box> ZONE_BOUNDING_BOXES = Map.of(
+            Zone.CRYSTAL_NUCLEUS, new Box(462, 63, 461, 564, 181, 565),
+            Zone.JUNGLE, new Box(201, 63, 201, 513, 189, 513),
+            Zone.MITHRIL_DEPOSITS, new Box(512, 63, 201, 824, 189, 513),
+            Zone.GOBLIN_HOLDOUT, new Box(201, 63, 512, 513, 189, 824),
+            Zone.PRECURSOR_REMNANTS, new Box(512, 63, 512, 824, 189, 824),
+            Zone.MAGMA_FIELDS, new Box(201, 30, 201, 824, 64, 824)
+    );
     private static final Vec3d JUNGLE_TEMPLE_DOOR_OFFSET = new Vec3d(-57, 36, -21);
     /**
-     * how many particles to use to get direction of a line
+     * The number of particles to use to get direction of a line
      */
     private static final long PARTICLES_PER_LINE = 25;
     /**
-     * the amount of milliseconds to wait for the next particle until assumed failed
+     * The time in milliseconds to wait for the next particle until assumed failed
      */
     private static final long PARTICLES_MAX_DELAY = 500;
     /**
@@ -93,12 +77,12 @@ public class WishingCompassSolver {
     }
 
     /**
-     * When a filed message is sent in chat reset the wishing compass solver to start
+     * When a filed message is sent in chat, reset the wishing compass solver to start
      * @param text message
      * @param b overlay
      */
     private static void failMessageListener(Text text, boolean b) {
-        if (!Utils.isInCrystalHollows()){
+        if (!Utils.isInCrystalHollows()) {
             return;
         }
         if (Formatting.strip(text.getString()).equals("The Wishing Compass can't seem to locate anything!")) {
@@ -115,7 +99,7 @@ public class WishingCompassSolver {
         particleUsedCountOne = 0;
         particleUsedCountTwo = 0;
         particleLastUpdate = System.currentTimeMillis();
-        Vec3d particleLastPos = Vec3d.ZERO;
+        particleLastPos = Vec3d.ZERO;
     }
 
     private static boolean isKingsScentPresent() {
@@ -127,45 +111,26 @@ public class WishingCompassSolver {
     }
 
     private static boolean isKeyInInventory() {
-        if (CLIENT.player == null) {
-            return false;
-        }
-        for (ItemStack item : CLIENT.player.getInventory().main) {
-            if (item != null && Objects.equals(item.getSkyblockId(), "JUNGLE_KEY")) {
-                return true;
-            }
-        }
-        return false;
+        return CLIENT.player != null && CLIENT.player.getInventory().main.stream().anyMatch(stack -> stack != null && Objects.equals(stack.getSkyblockId(), "JUNGLE_KEY"));
     }
 
-    private static ZONE getZoneOfLocation(Vec3d location) {
-        for (Map.Entry<ZONE, Box> zone : ZONE_BOUNDING_BOXES.entrySet()) {
-            if (zone.getValue().contains(location)) {
-                return zone.getKey();
-            }
-        }
-
-        //default to nucleus if somehow not in another zone
-        return ZONE.CRYSTAL_NUCLEUS;
+    private static Zone getZoneOfLocation(Vec3d location) {
+        return ZONE_BOUNDING_BOXES.entrySet().stream().filter(zone -> zone.getValue().contains(location)).findFirst().map(Map.Entry::getKey).orElse(Zone.CRYSTAL_NUCLEUS); //default to nucleus if somehow not in another zone
     }
 
-    private static Boolean isZoneComplete(ZONE zone) {
+    private static Boolean isZoneComplete(Zone zone) {
         if (CLIENT.getNetworkHandler() == null || CLIENT.player == null) {
             return false;
         }
-        //creates cleaned stream of all the entry's in tab list
-        Stream<PlayerListEntry> playerListStream = CLIENT.getNetworkHandler().getPlayerList().stream();
-        Stream<String> displayNameStream = playerListStream.map(PlayerListEntry::getDisplayName).filter(Objects::nonNull).map(Text::getString).map(String::strip);
 
         //make sure the data is in tab and if not tell the user
-        if (displayNameStream.noneMatch(entry -> entry.equals("Crystals:"))) {
+        if (PlayerListMgr.getPlayerStringList().stream().noneMatch(entry -> entry.equals("Crystals:"))) {
             CLIENT.player.sendMessage(Constants.PREFIX.get().append(Text.translatable("skyblocker.config.mining.crystalsWaypoints.wishingCompassSolver.enableTabMessage")), false);
             return false;
         }
 
         //return if the crystal for a zone is found
-        playerListStream = CLIENT.getNetworkHandler().getPlayerList().stream();
-        displayNameStream = playerListStream.map(PlayerListEntry::getDisplayName).filter(Objects::nonNull).map(Text::getString).map(String::strip);
+        Stream<String> displayNameStream = PlayerListMgr.getPlayerStringList().stream();
         return switch (zone) {
             case JUNGLE -> displayNameStream.noneMatch(entry -> entry.equals("Amethyst: ✖ Not Found"));
             case MITHRIL_DEPOSITS -> displayNameStream.noneMatch(entry -> entry.equals("Jade: ✖ Not Found"));
@@ -176,17 +141,15 @@ public class WishingCompassSolver {
         };
     }
 
-    private static MiningLocationLabel.CrystalHollowsLocationsCategory getTargetLocation(ZONE startingZone) {
+    private static MiningLocationLabel.CrystalHollowsLocationsCategory getTargetLocation(Zone startingZone) {
         //if the zone is complete return null
         if (isZoneComplete(startingZone)) {
             return MiningLocationLabel.CrystalHollowsLocationsCategory.UNKNOWN;
         }
         return switch (startingZone) {
-            case JUNGLE ->
-                    isKeyInInventory() ? MiningLocationLabel.CrystalHollowsLocationsCategory.JUNGLE_TEMPLE : MiningLocationLabel.CrystalHollowsLocationsCategory.ODAWA;
+            case JUNGLE -> isKeyInInventory() ? MiningLocationLabel.CrystalHollowsLocationsCategory.JUNGLE_TEMPLE : MiningLocationLabel.CrystalHollowsLocationsCategory.ODAWA;
             case MITHRIL_DEPOSITS -> MiningLocationLabel.CrystalHollowsLocationsCategory.MINES_OF_DIVAN;
-            case GOBLIN_HOLDOUT ->
-                    isKingsScentPresent() ? MiningLocationLabel.CrystalHollowsLocationsCategory.GOBLIN_QUEENS_DEN : MiningLocationLabel.CrystalHollowsLocationsCategory.KING_YOLKAR;
+            case GOBLIN_HOLDOUT -> isKingsScentPresent() ? MiningLocationLabel.CrystalHollowsLocationsCategory.GOBLIN_QUEENS_DEN : MiningLocationLabel.CrystalHollowsLocationsCategory.KING_YOLKAR;
             case PRECURSOR_REMNANTS -> MiningLocationLabel.CrystalHollowsLocationsCategory.LOST_PRECURSOR_CITY;
             case MAGMA_FIELDS -> MiningLocationLabel.CrystalHollowsLocationsCategory.KHAZAD_DUM;
             default -> MiningLocationLabel.CrystalHollowsLocationsCategory.UNKNOWN;
@@ -194,12 +157,12 @@ public class WishingCompassSolver {
     }
 
     /**
-     * Verify that a location could be correct and not to far out of zone. This is a problem when areas sometimes do not exist and is not a perfect solution
+     * Verifies that a location could be correct and not to far out of zone. This is a problem when areas sometimes do not exist and is not a perfect solution
      * @param startingZone zone player is searching in
      * @param pos location where the area should be
      * @return corrected location
      */
-    private static Boolean verifyLocation(ZONE startingZone, Vec3d pos) {
+    private static Boolean verifyLocation(Zone startingZone, Vec3d pos) {
         return ZONE_BOUNDING_BOXES.get(startingZone).expand(100, 0, 100).contains(pos);
     }
 
@@ -254,7 +217,7 @@ public class WishingCompassSolver {
             CLIENT.player.sendMessage(Constants.PREFIX.get().append(Text.translatable("skyblocker.config.mining.crystalsWaypoints.wishingCompassSolver.somethingWentWrongMessage").formatted(Formatting.RED)), false);
         } else {
             //send message to player with location and name
-            ZONE playerZone = getZoneOfLocation(startPosOne);
+            Zone playerZone = getZoneOfLocation(startPosOne);
             MiningLocationLabel.CrystalHollowsLocationsCategory location = getTargetLocation(playerZone);
             if (!verifyLocation(playerZone, targetLocation)) {
                 location = MiningLocationLabel.CrystalHollowsLocationsCategory.UNKNOWN;
@@ -342,12 +305,12 @@ public class WishingCompassSolver {
             return true;
         }
         Vec3d playerPos = CLIENT.player.getEyePos();
-        ZONE currentZone = getZoneOfLocation(playerPos);
+        Zone currentZone = getZoneOfLocation(playerPos);
 
         switch (currentState) {
             case NOT_STARTED -> {
                 //do not start if the player is in nucleus as this does not work well
-                if (currentZone == ZONE.CRYSTAL_NUCLEUS) {
+                if (currentZone == Zone.CRYSTAL_NUCLEUS) {
                     CLIENT.player.sendMessage(Constants.PREFIX.get().append(Text.translatable("skyblocker.config.mining.crystalsWaypoints.wishingCompassSolver.useOutsideNucleusMessage")), false);
                     return true;
                 }
@@ -407,4 +370,20 @@ public class WishingCompassSolver {
             particleLastPos = playerPos;
         }
     }
+
+    private enum SolverStates {
+        NOT_STARTED,
+        PROCESSING_FIRST_USE,
+        WAITING_FOR_SECOND,
+        PROCESSING_SECOND_USE,
+    }
+
+    private enum Zone {
+        CRYSTAL_NUCLEUS,
+        JUNGLE,
+        MITHRIL_DEPOSITS,
+        GOBLIN_HOLDOUT,
+        PRECURSOR_REMNANTS,
+        MAGMA_FIELDS,
+    }
 }
diff --git a/src/main/java/de/hysky/skyblocker/skyblock/tabhud/util/PlayerListMgr.java b/src/main/java/de/hysky/skyblocker/skyblock/tabhud/util/PlayerListMgr.java
index 472cf700..b08a09d6 100644
--- a/src/main/java/de/hysky/skyblocker/skyblock/tabhud/util/PlayerListMgr.java
+++ b/src/main/java/de/hysky/skyblocker/skyblock/tabhud/util/PlayerListMgr.java
@@ -1,19 +1,20 @@
 package de.hysky.skyblocker.skyblock.tabhud.util;
 
-import java.util.List;
-import java.util.regex.Matcher;
-import java.util.regex.Pattern;
-
 import de.hysky.skyblocker.mixins.accessors.PlayerListHudAccessor;
 import de.hysky.skyblocker.utils.Utils;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
-
 import net.minecraft.client.MinecraftClient;
 import net.minecraft.client.network.ClientPlayNetworkHandler;
 import net.minecraft.client.network.PlayerListEntry;
 import net.minecraft.text.MutableText;
 import net.minecraft.text.Text;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Objects;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
 
 /**
  * This class may be used to get data from the player list. It doesn't get its
@@ -24,8 +25,15 @@ public class PlayerListMgr {
 
 	public static final Logger LOGGER = LoggerFactory.getLogger("Skyblocker Regex");
 
-	private static List<PlayerListEntry> playerList;
-    private static String footer;
+	/**
+	 * The player list in tab.
+	 */
+	private static List<PlayerListEntry> playerList = new ArrayList<>(); // Initialize to prevent npe.
+	/**
+	 * The player list in tab, but a list of strings instead of {@link PlayerListEntry}s.
+	 */
+	private static List<String> playerStringList = new ArrayList<>();
+	private static String footer;
 
 	public static void updateList() {
 
@@ -38,25 +46,40 @@ public class PlayerListMgr {
 		// check is needed, else game crashes on server leave
 		if (cpnwh != null) {
 			playerList = cpnwh.getPlayerList().stream().sorted(PlayerListHudAccessor.getOrdering()).toList();
+			playerStringList = playerList.stream().map(PlayerListEntry::getDisplayName).filter(Objects::nonNull).map(Text::getString).map(String::strip).toList();
 		}
 	}
 
-    public static void updateFooter(Text f) {
-        if (f == null) {
-            footer = null;
-        } else {
-            footer = f.getString();
-        }
-    }
+	/**
+	 * @return the cached player list
+	 */
+	public static List<PlayerListEntry> getPlayerList() {
+		return playerList;
+	}
+
+	/**
+	 * @return the cached player list as a list of strings
+	 */
+	public static List<String> getPlayerStringList() {
+		return playerStringList;
+	}
+
+	public static void updateFooter(Text f) {
+		if (f == null) {
+			footer = null;
+		} else {
+			footer = f.getString();
+		}
+	}
 
-    public static String getFooter() {
-        return footer;
-    }
+	public static String getFooter() {
+		return footer;
+	}
 
 	/**
 	 * Get the display name at some index of the player list and apply a pattern to
 	 * it
-	 * 
+	 *
 	 * @return the matcher if p fully matches, else null
 	 */
 	public static Matcher regexAt(int idx, Pattern p) {
@@ -78,7 +101,7 @@ public class PlayerListMgr {
 
 	/**
 	 * Get the display name at some index of the player list as string
-	 * 
+	 *
 	 * @return the string or null, if the display name is null, empty or whitespace
 	 *         only
 	 */
@@ -105,9 +128,9 @@ public class PlayerListMgr {
 
 	/**
 	 * Gets the display name at some index of the player list
-	 * 
+	 *
 	 * @return the text or null, if the display name is null
-	 * 
+	 *
 	 * @implNote currently designed specifically for crimson isles faction quests
 	 *           widget and the rift widgets, might not work correctly without
 	 *           modification for other stuff. you've been warned!
@@ -157,7 +180,7 @@ public class PlayerListMgr {
 	/**
 	 * Get the display name at some index of the player list as Text as seen in the
 	 * game
-	 * 
+	 *
 	 * @return the PlayerListEntry at that index
 	 */
 	public static PlayerListEntry getRaw(int idx) {