Refactor: Fix Infrastructure and Design Code Smells for Improved Maintainability

worldedit-core/src/main/java/com/sk89q/worldedit/LocalSession.java

@@ -49,6 +49,7 @@
 import com.sk89q.worldedit.session.ClipboardHolder;
 import com.sk89q.worldedit.session.Placement;
 import com.sk89q.worldedit.session.PlacementType;
+import com.sk89q.worldedit.session.SessionCUIState;
 import com.sk89q.worldedit.session.request.Request;
 import com.sk89q.worldedit.util.Countable;
 import com.sk89q.worldedit.util.SideEffectSet;
@@ -84,17 +85,14 @@
  */
 public class LocalSession {
 
-    private static final int CUI_VERSION_UNINITIALIZED = -1;
     public static int MAX_HISTORY_SIZE = 15;
 
     // Non-session related fields
     private transient LocalConfiguration config;
     private final transient AtomicBoolean dirty = new AtomicBoolean();
 
-    // Single-connection lifetime fields
-    private transient int failedCuiAttempts = 0;
-    private transient boolean hasCUISupport = false;
-    private transient int cuiVersion = CUI_VERSION_UNINITIALIZED;
+    // CUI state (extracted to SessionCUIState - move field)
+    private final transient SessionCUIState cuiState = new SessionCUIState();
 
     // Session related
     private transient RegionSelector selector = new CuboidRegionSelector();
@@ -103,7 +101,7 @@ public class LocalSession {
     private transient int historyPointer = 0;
     private transient ClipboardHolder clipboard;
     private transient boolean superPickaxe = false;
-    private transient BlockTool pickaxeMode = new SinglePickaxe();
+    private transient BlockTool superPickaxeTool = new SinglePickaxe();
     private final transient Map<ItemType, Tool> tools = new HashMap<>();
     private transient int maxBlocksChanged = -1;
     private transient int maxTimeoutTime;
@@ -113,7 +111,6 @@ public class LocalSession {
     private transient SideEffectSet sideEffectSet = SideEffectSet.defaults();
     private transient Mask mask;
     private transient ZoneId timezone = ZoneId.systemDefault();
-    private transient BlockVector3 cuiTemporaryBlock;
     @SuppressWarnings("deprecation")
     private transient EditSession.ReorderMode reorderMode = EditSession.ReorderMode.FAST;
     private transient List<Countable<BlockState>> lastDistribution;
@@ -428,13 +425,25 @@ public Region getSelection() throws IncompleteRegionException {
      * @throws IncompleteRegionException if no region is selected, or the provided world is null
      */
     public Region getSelection(@Nullable World world) throws IncompleteRegionException {
-        if (world == null || selector.getIncompleteRegion().getWorld() == null
-            || !selector.getIncompleteRegion().getWorld().equals(world)) {
+        if (!isSelectionDefinedForWorld(world)) {
             throw new IncompleteRegionException();
         }
         return selector.getRegion();
     }
 
+    /**
+     * Returns whether the selection is fully defined for the given world.
+     * Decomposed conditional for clarity (code smell: complex conditional).
+     */
+    private boolean isSelectionDefinedForWorld(@Nullable World world) {
+        if (world == null) {
+            return false;
+        }
+        World selectionWorld = selector.getIncompleteRegion().getWorld();
+        boolean selectionMatchesWorld = selectionWorld != null && selectionWorld.equals(world);
+        return selectionMatchesWorld;
+    }
+
     /**
      * Get the selection world.
      *
@@ -699,7 +708,7 @@ public void setSnapshotExperimental(@Nullable SnapshotInfo snapshotExperimental)
      * @return the super pickaxe tool mode
      */
     public BlockTool getSuperPickaxe() {
-        return pickaxeMode;
+        return superPickaxeTool;
     }
 
     /**
@@ -709,7 +718,7 @@ public BlockTool getSuperPickaxe() {
      */
     public void setSuperPickaxe(BlockTool tool) {
         checkNotNull(tool);
-        this.pickaxeMode = tool;
+        this.superPickaxeTool = tool;
     }
 
     /**
@@ -899,10 +908,11 @@ public void updateServerCUI(Actor actor) {
 
         Player player = (Player) actor;
 
-        if (!useServerCUI || hasCUISupport) {
-            if (cuiTemporaryBlock != null) {
-                player.sendFakeBlock(cuiTemporaryBlock, null);
-                cuiTemporaryBlock = null;
+        if (!useServerCUI || cuiState.hasCUISupport()) {
+            BlockVector3 pos = cuiState.getServerCuiStructureBlockPosition();
+            if (pos != null) {
+                player.sendFakeBlock(pos, null);
+                cuiState.setServerCuiStructureBlockPosition(null);
             }
             return; // If it's not enabled, ignore this.
         }
@@ -912,22 +922,23 @@ public void updateServerCUI(Actor actor) {
             LinCompoundTag tags = Objects.requireNonNull(
                 block.getNbt(), "createStructureBlock should return nbt"
             );
-            BlockVector3 tempCuiTemporaryBlock = BlockVector3.at(
+            BlockVector3 newStructureBlockPosition = BlockVector3.at(
                 tags.getTag("x", LinTagType.intTag()).valueAsInt(),
                 tags.getTag("y", LinTagType.intTag()).valueAsInt(),
                 tags.getTag("z", LinTagType.intTag()).valueAsInt()
             );
-            // If it's null, we don't need to do anything. The old was already removed.
-            if (cuiTemporaryBlock != null && !tempCuiTemporaryBlock.equals(cuiTemporaryBlock)) {
-                // Update the existing block if it's the same location
-                player.sendFakeBlock(cuiTemporaryBlock, null);
+            BlockVector3 currentPos = cuiState.getServerCuiStructureBlockPosition();
+            if (currentPos != null && !newStructureBlockPosition.equals(currentPos)) {
+                player.sendFakeBlock(currentPos, null);
+            }
+            cuiState.setServerCuiStructureBlockPosition(newStructureBlockPosition);
+            player.sendFakeBlock(newStructureBlockPosition, block);
+        } else {
+            BlockVector3 pos = cuiState.getServerCuiStructureBlockPosition();
+            if (pos != null) {
+                player.sendFakeBlock(pos, null);
+                cuiState.setServerCuiStructureBlockPosition(null);
             }
-            cuiTemporaryBlock = tempCuiTemporaryBlock;
-            player.sendFakeBlock(cuiTemporaryBlock, block);
-        } else if (cuiTemporaryBlock != null) {
-            // Remove the old block
-            player.sendFakeBlock(cuiTemporaryBlock, null);
-            cuiTemporaryBlock = null;
         }
     }
 
@@ -941,7 +952,7 @@ public void dispatchCUIEvent(Actor actor, CUIEvent event) {
         checkNotNull(actor);
         checkNotNull(event);
 
-        if (hasCUISupport) {
+        if (cuiState.hasCUISupport()) {
             actor.dispatchCUIEvent(event);
         } else if (useServerCUI) {
             updateServerCUI(actor);
@@ -967,15 +978,15 @@ public void dispatchCUISetup(Actor actor) {
     public void dispatchCUISelection(Actor actor) {
         checkNotNull(actor);
 
-        if (!hasCUISupport) {
+        if (!cuiState.hasCUISupport()) {
             if (useServerCUI) {
                 updateServerCUI(actor);
             }
             return;
         }
 
         if (selector instanceof CUIRegion tempSel) {
-            if (tempSel.getProtocolVersion() > cuiVersion) {
+            if (tempSel.getProtocolVersion() > cuiState.getCUIVersion()) {
                 actor.dispatchCUIEvent(new SelectionShapeEvent(tempSel.getLegacyTypeID()));
                 tempSel.describeLegacyCUI(this, actor);
             } else {
@@ -994,12 +1005,12 @@ public void dispatchCUISelection(Actor actor) {
     public void describeCUI(Actor actor) {
         checkNotNull(actor);
 
-        if (!hasCUISupport) {
+        if (!cuiState.hasCUISupport()) {
             return;
         }
 
         if (selector instanceof CUIRegion tempSel) {
-            if (tempSel.getProtocolVersion() > cuiVersion) {
+            if (tempSel.getProtocolVersion() > cuiState.getCUIVersion()) {
                 tempSel.describeLegacyCUI(this, actor);
             } else {
                 tempSel.describeCUI(this, actor);
@@ -1019,18 +1030,18 @@ public void handleCUIInitializationMessage(String eventType, List<String> args,
         checkNotNull(eventType);
         checkNotNull(args);
 
-        if (this.hasCUISupport) {
+        if (cuiState.hasCUISupport()) {
             // WECUI is a bit aggressive about re-initializing itself
             // the last attempt to touch handshakes didn't go well, so this will do... for now
             dispatchCUISelection(actor);
             return;
-        } else if (this.failedCuiAttempts > 3) {
+        } else if (cuiState.getFailedCuiAttempts() > 3) {
             return;
         }
 
         if (!args.isEmpty() && eventType.equalsIgnoreCase("v")) { // enough fields and right message
             if (args.size() > 1) {
-                this.failedCuiAttempts++;
+                cuiState.incrementFailedCuiAttempts();
                 return;
             }
 
@@ -1039,11 +1050,11 @@ public void handleCUIInitializationMessage(String eventType, List<String> args,
                 version = Integer.parseInt(args.getFirst());
             } catch (NumberFormatException e) {
                 WorldEdit.logger.warn("Error while reading CUI init message");
-                this.failedCuiAttempts++;
+                cuiState.incrementFailedCuiAttempts();
                 return;
             }
-            setCUISupport(true);
-            setCUIVersion(version);
+            cuiState.setCUISupport(true);
+            cuiState.setCUIVersion(version);
             dispatchCUISelection(actor);
         }
     }
@@ -1070,7 +1081,7 @@ public void handleCUIInitializationMessage(String text, Actor actor) {
      * @return true if CUI is enabled
      */
     public boolean hasCUISupport() {
-        return hasCUISupport;
+        return cuiState.hasCUISupport();
     }
 
     /**
@@ -1079,7 +1090,7 @@ public boolean hasCUISupport() {
      * @param support true if CUI is enabled
      */
     public void setCUISupport(boolean support) {
-        hasCUISupport = support;
+        cuiState.setCUISupport(support);
     }
 
     /**
@@ -1088,7 +1099,7 @@ public void setCUISupport(boolean support) {
      * @return the CUI version
      */
     public int getCUIVersion() {
-        return cuiVersion;
+        return cuiState.getCUIVersion();
     }
 
     /**
@@ -1097,11 +1108,7 @@ public int getCUIVersion() {
      * @param cuiVersion the CUI version
      */
     public void setCUIVersion(int cuiVersion) {
-        if (cuiVersion < 0) {
-            throw new IllegalArgumentException("CUI protocol version must be non-negative, but '" + cuiVersion + "' was received.");
-        }
-
-        this.cuiVersion = cuiVersion;
+        cuiState.setCUIVersion(cuiVersion);
     }
 
     /**
@@ -1314,8 +1321,6 @@ public void setLastDistribution(List<Countable<BlockState>> dist) {
      * <p>This is for internal use only.</p>
      */
     public void onIdle() {
-        this.cuiVersion = CUI_VERSION_UNINITIALIZED;
-        this.hasCUISupport = false;
-        this.failedCuiAttempts = 0;
+        cuiState.reset();
     }
 }

worldedit-core/src/main/java/com/sk89q/worldedit/session/SessionCUIState.java

@@ -0,0 +1,88 @@
+/*
+ * WorldEdit, a Minecraft world manipulation toolkit
+ * Copyright (C) sk89q <http://www.sk89q.com>
+ * Copyright (C) WorldEdit team and contributors
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <https://www.gnu.org/licenses/>.
+ */
+
+package com.sk89q.worldedit.session;
+
+import com.sk89q.worldedit.math.BlockVector3;
+
+import javax.annotation.Nullable;
+
+/**
+ * Holds CUI (CUINetworking)-related state for a session.
+ * Extracted from LocalSession to separate concerns (code smell: large class with mixed responsibilities).
+ */
+public final class SessionCUIState {
+
+    public static final int CUI_VERSION_UNINITIALIZED = -1;
+
+    private int failedCuiAttempts = 0;
+    private boolean hasCUISupport = false;
+    private int cuiVersion = CUI_VERSION_UNINITIALIZED;
+    @Nullable
+    private BlockVector3 serverCuiStructureBlockPosition;
+
+    public int getFailedCuiAttempts() {
+        return failedCuiAttempts;
+    }
+
+    public void setFailedCuiAttempts(int failedCuiAttempts) {
+        this.failedCuiAttempts = failedCuiAttempts;
+    }
+
+    public void incrementFailedCuiAttempts() {
+        this.failedCuiAttempts++;
+    }
+
+    public boolean hasCUISupport() {
+        return hasCUISupport;
+    }
+
+    public void setCUISupport(boolean support) {
+        this.hasCUISupport = support;
+    }
+
+    public int getCUIVersion() {
+        return cuiVersion;
+    }
+
+    public void setCUIVersion(int cuiVersion) {
+        if (cuiVersion < 0) {
+            throw new IllegalArgumentException("CUI protocol version must be non-negative, but '" + cuiVersion + "' was received.");
+        }
+        this.cuiVersion = cuiVersion;
+    }
+
+    @Nullable
+    public BlockVector3 getServerCuiStructureBlockPosition() {
+        return serverCuiStructureBlockPosition;
+    }
+
+    public void setServerCuiStructureBlockPosition(@Nullable BlockVector3 position) {
+        this.serverCuiStructureBlockPosition = position;
+    }
+
+    /**
+     * Reset CUI state when the session becomes idle.
+     */
+    public void reset() {
+        this.cuiVersion = CUI_VERSION_UNINITIALIZED;
+        this.hasCUISupport = false;
+        this.failedCuiAttempts = 0;
+    }
+}