refactor: reduce cognitive complexity (SonarCloud S3776) for 13 quick-win methods

Author: MPCoreDeveloper

src/SharpCoreDB/DataStructures/BTree.cs

@@ -366,60 +366,79 @@ private bool DeleteFromNode(Node node, TKey key)
                 return true;
             }
 
-            // Internal (separator) node: values live in leaves, so this node only routes ranges.
-            // Removing the separator outright would leave the child-pointer ↔ separator mapping
-            // inconsistent (keys between the deleted separator and the next one would become
-            // unreachable), so the separator is replaced by its in-order successor taken from the
-            // right subtree's leftmost leaf, and that leaf entry is then deleted recursively.
-            var successorChild = node.childrenArray[i + 1];
-            while (!successorChild.IsLeaf)
-            {
-                successorChild = successorChild.childrenArray[0];
-            }
+            return DeleteSeparatorFromInternalNode(node, i);
+        }
 
-            if (successorChild.keysCount > 0)
-            {
-                node.keysArray[i] = successorChild.keysArray[0];
-                if (i < node.valuesCount)
-                {
-                    node.valuesArray[i] = successorChild.valuesArray[0];
-                }
+        if (!node.IsLeaf)
+        {
+            return DeleteFromNode(node.childrenArray[i], key);
+        }
 
-                return DeleteFromNode(node.childrenArray[i + 1], node.keysArray[i]);
-            }
+        return false;
+    }
 
-            // The right subtree is empty (fully drained) — fall back to the left subtree's maximum
-            // when it still holds entries.
-            var predecessorChild = node.childrenArray[i];
-            while (!predecessorChild.IsLeaf)
+    /// <summary>
+    /// Deletes the separator at position <paramref name="i"/> of an internal (routing) node.
+    /// Since values live in leaves, the separator is replaced by its in-order successor taken
+    /// from the right subtree's leftmost leaf (which is then deleted recursively). When that
+    /// subtree is fully drained, the left subtree's maximum is used instead. Only when both
+    /// neighbour subtrees are exhausted is the separator dropped together with its empty right
+    /// child, so the child-pointer ↔ separator mapping stays consistent.
+    /// </summary>
+    private bool DeleteSeparatorFromInternalNode(Node node, int i)
+    {
+        var successorChild = FindLeftmostLeaf(node.childrenArray[i + 1]);
+        if (successorChild.keysCount > 0)
+        {
+            node.keysArray[i] = successorChild.keysArray[0];
+            if (i < node.valuesCount)
             {
-                predecessorChild = predecessorChild.childrenArray[predecessorChild.childrenCount - 1];
+                node.valuesArray[i] = successorChild.valuesArray[0];
             }
 
-            if (predecessorChild.keysCount > 0)
-            {
-                int predPos = predecessorChild.keysCount - 1;
-                node.keysArray[i] = predecessorChild.keysArray[predPos];
-                if (i < node.valuesCount)
-                {
-                    node.valuesArray[i] = predecessorChild.valuesArray[predPos];
-                }
+            return DeleteFromNode(node.childrenArray[i + 1], node.keysArray[i]);
+        }
 
-                return DeleteFromNode(node.childrenArray[i], node.keysArray[i]);
+        // The right subtree is empty (fully drained) — fall back to the left subtree's maximum
+        // when it still holds entries.
+        var predecessorChild = FindRightmostLeaf(node.childrenArray[i]);
+        if (predecessorChild.keysCount > 0)
+        {
+            int predPos = predecessorChild.keysCount - 1;
+            node.keysArray[i] = predecessorChild.keysArray[predPos];
+            if (i < node.valuesCount)
+            {
+                node.valuesArray[i] = predecessorChild.valuesArray[predPos];
             }
 
-            // Both neighbour subtrees are drained — drop the separator together with its empty
-            // right child so the child pointer count stays consistent with the key count.
-            RemoveKeyAt(node, i);
-            RemoveChildAt(node, i + 1);
-            return true;
+            return DeleteFromNode(node.childrenArray[i], node.keysArray[i]);
         }
-        else if (!node.IsLeaf)
+
+        // Both neighbour subtrees are drained — drop the separator together with its empty
+        // right child so the child pointer count stays consistent with the key count.
+        RemoveKeyAt(node, i);
+        RemoveChildAt(node, i + 1);
+        return true;
+    }
+
+    private static Node FindLeftmostLeaf(Node node)
+    {
+        while (!node.IsLeaf)
         {
-            return DeleteFromNode(node.childrenArray[i], key);
+            node = node.childrenArray[0];
         }
 
-        return false;
+        return node;
+    }
+
+    private static Node FindRightmostLeaf(Node node)
+    {
+        while (!node.IsLeaf)
+        {
+            node = node.childrenArray[node.childrenCount - 1];
+        }
+
+        return node;
     }
 
     private static void RemoveChildAt(Node node, int pos)

src/SharpCoreDB/DataStructures/HashIndex.cs

@@ -429,47 +429,57 @@ internal void AddBatchKeys(object?[] keys, long[] positions)
         _lock.EnterWriteLock();
         try
         {
-            for (int i = 0; i < keys.Length; i++)
-            {
-                if (keys[i] is null)
-                {
-                    continue;
-                }
-
-                var normalizedKey = NormalizeKey(keys[i]);
+            AddBatchKeysLockedCore(keys, positions);
+        }
+        finally
+        {
+            _lock.ExitWriteLock();
+        }
+    }
 
-                if (_useUnsafeEqualityIndex)
-                {
-                    // Unique path: atomic check + add under outer lock.
-                    var keyBytes = BuildUnsafeKey(normalizedKey);
-                    if (HasUnsafeRowsForKey(keyBytes))
-                    {
-                        throw new InvalidOperationException(
-                            $"Duplicate key value '{keys[i]}' violates unique constraint on index '{_columnName}'");
-                    }
+    /// <summary>
+    /// Per-key insert loop for the batched add, executed under the outer write lock. Handles the
+    /// unique (unsafe equality index) path via an atomic per-key check-and-add and the regular
+    /// dictionary path via list append; duplicate keys on a unique index throw.
+    /// </summary>
+    private void AddBatchKeysLockedCore(object?[] keys, long[] positions)
+    {
+        for (int i = 0; i < keys.Length; i++)
+        {
+            if (keys[i] is null)
+            {
+                continue;
+            }
 
-                    _unsafeIndex.Add(keyBytes, positions[i]);
-                    _unsafeTotalRows++;
-                    continue;
-                }
+            var normalizedKey = NormalizeKey(keys[i]);
 
-                if (!_index.TryGetValue(normalizedKey, out var list))
-                {
-                    list = [];
-                    _index[normalizedKey] = list;
-                }
-                else if (_isUnique && list.Count > 0)
+            if (_useUnsafeEqualityIndex)
+            {
+                // Unique path: atomic check + add under outer lock.
+                var keyBytes = BuildUnsafeKey(normalizedKey);
+                if (HasUnsafeRowsForKey(keyBytes))
                 {
                     throw new InvalidOperationException(
                         $"Duplicate key value '{keys[i]}' violates unique constraint on index '{_columnName}'");
                 }
 
-                list.Add(positions[i]);
+                _unsafeIndex.Add(keyBytes, positions[i]);
+                _unsafeTotalRows++;
+                continue;
             }
-        }
-        finally
-        {
-            _lock.ExitWriteLock();
+
+            if (!_index.TryGetValue(normalizedKey, out var list))
+            {
+                list = [];
+                _index[normalizedKey] = list;
+            }
+            else if (_isUnique && list.Count > 0)
+            {
+                throw new InvalidOperationException(
+                    $"Duplicate key value '{keys[i]}' violates unique constraint on index '{_columnName}'");
+            }
+
+            list.Add(positions[i]);
         }
     }
 

src/SharpCoreDB/DataStructures/Table.CRUD.cs

@@ -3420,37 +3420,50 @@ private bool IsFilePkOrderedUpTo(byte[] wholeFile, string pkCol, long firstSearc
         long prev = long.MinValue;
         while (walk + 4 <= wholeFile.Length && walk < firstSearch)
         {
-            int len = BinaryPrimitives.ReadInt32LittleEndian(wholeFile.AsSpan((int)walk, 4));
-            if (len > 0 && walk + 4 + len <= wholeFile.Length)
+            if (!TryAdvancePkOrderedWalk(wholeFile, pkWantedPre, pkCol, ref walk, ref prev))
             {
-                var r = DeserializeDeleteKeyRow(wholeFile.AsSpan((int)walk + 4, len), pkWantedPre);
-                if (r != null && r.TryGetValue(pkCol, out var v) && v is not null && v is not DBNull)
-                {
-                    long pk = Convert.ToInt64(v, CultureInfo.InvariantCulture);
-                    if (pk < prev)
-                    {
-                        return false; // physically unordered file -> per-row resolution
-                    }
+                return false; // physically unordered or malformed file -> per-row resolution
+            }
+        }
 
-                    prev = pk;
-                }
+        return true;
+    }
 
-                walk += 4 + len;
-            }
-            else
+    /// <summary>
+    /// Advances <paramref name="walk"/> over one physical record (or tombstone marker) of a
+    /// variable-length file, verifying the decoded PK keeps ascending order. Returns false when
+    /// the file is malformed (a zero length) or proves physically unordered.
+    /// </summary>
+    private bool TryAdvancePkOrderedWalk(byte[] wholeFile, int[] pkWanted, string pkCol, ref long walk, ref long prev)
+    {
+        int len = BinaryPrimitives.ReadInt32LittleEndian(wholeFile.AsSpan((int)walk, 4));
+        if (len > 0 && walk + 4 + len <= wholeFile.Length)
+        {
+            var r = DeserializeDeleteKeyRow(wholeFile.AsSpan((int)walk + 4, len), pkWanted);
+            if (r != null && r.TryGetValue(pkCol, out var v) && v is not null && v is not DBNull)
             {
-                if (len == 0)
+                long pk = Convert.ToInt64(v, CultureInfo.InvariantCulture);
+                if (pk < prev)
                 {
-                    return false;
+                    return false; // physically unordered file -> per-row resolution
                 }
 
-                // Tombstone marker: the negative value already encodes the whole slot span
-                // (4-byte prefix + payload), so skipping by |len| lands exactly on the next
-                // record's prefix.
-                walk += Math.Abs(len);
+                prev = pk;
             }
+
+            walk += 4 + len;
+            return true;
+        }
+
+        if (len == 0)
+        {
+            return false;
         }
 
+        // Tombstone marker: the negative value already encodes the whole slot span
+        // (4-byte prefix + payload), so skipping by |len| lands exactly on the next
+        // record's prefix.
+        walk += Math.Abs(len);
         return true;
     }
 

src/SharpCoreDB/DataStructures/Table.FixedWidthMigration.cs

@@ -96,34 +96,7 @@ public int MigrateToFixedWidth()
             // 5. Write the new records to a temp file and swap it in atomically (the same pattern
             //    as AppendOnlyEngine.CompactTable, so encryption handling is identical).
             var tempPath = DataFile + ".fwmig.tmp";
-            try
-            {
-                if (File.Exists(tempPath))
-                {
-                    File.Delete(tempPath);
-                }
-
-                if (records.Count > 0)
-                {
-                    storage.AppendBytesMultiple(tempPath, records);
-                }
-                else
-                {
-                    File.WriteAllBytes(tempPath, Array.Empty<byte>());
-                }
-
-                File.Delete(DataFile);
-                File.Move(tempPath, DataFile);
-            }
-            catch
-            {
-                if (File.Exists(tempPath))
-                {
-                    try { File.Delete(tempPath); } catch { /* best-effort cleanup */ }
-                }
-
-                throw;
-            }
+            WriteMigratedRecordsAndSwap(tempPath, records);
 
             // 6. Rebuild the indexes against the new fixed-width records (DeserializeRow dispatches
             //    to the fixed-width codec now) and fix the cached row count.
@@ -143,6 +116,43 @@ public int MigrateToFixedWidth()
         }
     }
 
+    /// <summary>
+    /// Writes the migrated fixed-width records to <paramref name="tempPath"/> and atomically swaps
+    /// it over <see cref="DataFile"/>. Any leftover temp file from an earlier failed migration is
+    /// removed first; on failure the temp file is cleaned up and the exception re-thrown.
+    /// </summary>
+    private void WriteMigratedRecordsAndSwap(string tempPath, List<byte[]> records)
+    {
+        try
+        {
+            if (File.Exists(tempPath))
+            {
+                File.Delete(tempPath);
+            }
+
+            if (records.Count > 0)
+            {
+                storage.AppendBytesMultiple(tempPath, records);
+            }
+            else
+            {
+                File.WriteAllBytes(tempPath, Array.Empty<byte>());
+            }
+
+            File.Delete(DataFile);
+            File.Move(tempPath, DataFile);
+        }
+        catch
+        {
+            if (File.Exists(tempPath))
+            {
+                try { File.Delete(tempPath); } catch { /* best-effort cleanup */ }
+            }
+
+            throw;
+        }
+    }
+
     /// <summary>
     /// Converts this table from page-based to columnar (append-only) storage in place: the
     /// page-based engine and its <c>.pages</c> files are dropped, <see cref="StorageMode"/> is set