Address code review: extract magic numbers as named constants, improve variable names

Copilot · jongalloway · web-flow · commit 5c295966d204 · 2026-03-23T08:03:21.000Z
Co-authored-by: jongalloway <68539+jongalloway@users.noreply.github.com> Agent-Logs-Url: https://github.com/jongalloway/DiagramForge/sessions/944314c7-50c9-4e41-9fb1-1781e58761f5
diff --git a/src/DiagramForge/Layout/DefaultLayoutEngine.Wireframe.cs b/src/DiagramForge/Layout/DefaultLayoutEngine.Wireframe.cs
@@ -12,13 +12,19 @@ public sealed partial class DefaultLayoutEngine
     private const double WireframeContainerPad = 12.0;
     private const double WireframeDefaultWidth = 320.0;
     private const double WireframeButtonHeight = 34.0;
+    private const double WireframeButtonMinWidthRatio = 0.4;   // fraction of DefaultWidth
+    private const double WireframeButtonHPad = 32.0;           // horizontal text padding in button
     private const double WireframeInputHeight = 32.0;
     private const double WireframeControlHeight = 22.0;
+    private const double WireframeControlLabelGap = 8.0;       // space between control and its label
+    private const double WireframeControlMinWidth = 120.0;     // minimum width for checkbox/radio
     private const double WireframeToggleWidth = 48.0;
     private const double WireframeToggleHeight = 24.0;
     private const double WireframeTabBarHeight = 38.0;
     private const double WireframeDividerHeight = 4.0;
     private const double WireframeBadgeHeight = 20.0;
+    private const double WireframeBadgeMinWidth = 36.0;        // minimum badge pill width
+    private const double WireframeBadgeHPad = 16.0;            // horizontal padding inside badge
     private const double WireframeImageHeight = 100.0;
     private const double WireframeImageWidth = 160.0;
     private const double WireframeH1FontSize = 20.0;
@@ -136,7 +142,7 @@ private static void SizeWireframeLeaf(Node node, Theme theme)
             case "button":
             {
                 double textW = SvgRenderSupport.EstimateTextWidth(node.Label.Text, theme.FontSize);
-                node.Width = Math.Max(WireframeDefaultWidth * 0.4, textW + 32);
+                node.Width = Math.Max(WireframeDefaultWidth * WireframeButtonMinWidthRatio, textW + WireframeButtonHPad);
                 node.Height = WireframeButtonHeight;
                 break;
             }
@@ -149,15 +155,15 @@ private static void SizeWireframeLeaf(Node node, Theme theme)
             case "radio":
             {
                 double textW = SvgRenderSupport.EstimateTextWidth(node.Label.Text, theme.FontSize);
-                node.Width = Math.Max(120, WireframeControlHeight + 8 + textW);
+                node.Width = Math.Max(WireframeControlMinWidth, WireframeControlHeight + WireframeControlLabelGap + textW);
                 node.Height = WireframeControlHeight;
                 break;
             }
             case "toggle":
                 node.Width = WireframeToggleWidth
                     + (string.IsNullOrWhiteSpace(node.Label.Text)
                         ? 0
-                        : 8 + SvgRenderSupport.EstimateTextWidth(node.Label.Text, theme.FontSize));
+                        : WireframeControlLabelGap + SvgRenderSupport.EstimateTextWidth(node.Label.Text, theme.FontSize));
                 node.Height = WireframeToggleHeight;
                 break;
 
@@ -175,7 +181,7 @@ private static void SizeWireframeLeaf(Node node, Theme theme)
             {
                 double badgeFontSize = theme.FontSize * 0.8;
                 double textW = SvgRenderSupport.EstimateTextWidth(node.Label.Text, badgeFontSize);
-                node.Width = Math.Max(36, textW + 16);
+                node.Width = Math.Max(WireframeBadgeMinWidth, textW + WireframeBadgeHPad);
                 node.Height = WireframeBadgeHeight;
                 break;
             }
diff --git a/src/DiagramForge/Parsers/Wireframe/WireframeDslParser.cs b/src/DiagramForge/Parsers/Wireframe/WireframeDslParser.cs
@@ -290,8 +290,12 @@ private static bool IsContainerEnd(string line)
         if (string.IsNullOrEmpty(text))
             return null;
 
-        // Strip **bold** markers for label text (bold styling comes from wireframe:bold metadata)
-        bool isBold = text.StartsWith("**", StringComparison.Ordinal) && text.EndsWith("**", StringComparison.Ordinal) && text.Length > 4;
+        // Strip **bold** markers for label text (bold styling comes from wireframe:bold metadata).
+        // Minimum: "**x**" = 5 chars: 2 open markers + 1 body char + 2 close markers.
+        const int BoldWrapperLength = 4; // "**" prefix + "**" suffix
+        bool isBold = text.StartsWith("**", StringComparison.Ordinal)
+                      && text.EndsWith("**", StringComparison.Ordinal)
+                      && text.Length > BoldWrapperLength;
         if (isBold)
             text = text[2..^2].Trim();
 
@@ -304,20 +308,27 @@ private static bool IsContainerEnd(string line)
 
     private static Node? ParseBracketComponent(string line, string nodeId)
     {
-        // [on] / [off] → Toggle
-        if (string.Equals(line, "[on]", StringComparison.OrdinalIgnoreCase)
-            || (line.Length > 4 && string.Equals(line[..4], "[on]", StringComparison.OrdinalIgnoreCase) && line[4] == ' '))
+        // [on] / [off] → Toggle (case-insensitive, optional trailing label)
+        const string OnToken = "[on]";
+        const string OffToken = "[off]";
+
+        if (string.Equals(line, OnToken, StringComparison.OrdinalIgnoreCase)
+            || (line.Length > OnToken.Length
+                && string.Equals(line[..OnToken.Length], OnToken, StringComparison.OrdinalIgnoreCase)
+                && line[OnToken.Length] == ' '))
         {
-            var label = line.Length > 5 ? line[5..].Trim() : string.Empty;
+            var label = line.Length > OnToken.Length + 1 ? line[(OnToken.Length + 1)..].Trim() : string.Empty;
             var n = new Node(nodeId, label);
             n.Metadata["wireframe:kind"] = "toggle";
             n.Metadata["wireframe:on"] = true;
             return n;
         }
-        if (string.Equals(line, "[off]", StringComparison.OrdinalIgnoreCase)
-            || (line.Length > 5 && string.Equals(line[..5], "[off]", StringComparison.OrdinalIgnoreCase) && line[5] == ' '))
+        if (string.Equals(line, OffToken, StringComparison.OrdinalIgnoreCase)
+            || (line.Length > OffToken.Length
+                && string.Equals(line[..OffToken.Length], OffToken, StringComparison.OrdinalIgnoreCase)
+                && line[OffToken.Length] == ' '))
         {
-            var label = line.Length > 6 ? line[6..].Trim() : string.Empty;
+            var label = line.Length > OffToken.Length + 1 ? line[(OffToken.Length + 1)..].Trim() : string.Empty;
             var n = new Node(nodeId, label);
             n.Metadata["wireframe:kind"] = "toggle";
             n.Metadata["wireframe:on"] = false;
diff --git a/src/DiagramForge/Rendering/SvgNodeWriter.cs b/src/DiagramForge/Rendering/SvgNodeWriter.cs
@@ -765,6 +765,8 @@ private static void AppendClassNode(
     private const string WfCheckColor = "#2A2A2A";
     private const double WfStroke = 1.2;
     private const double WfRadius = 4.0;
+    private const double WfCheckmarkInsetRatio = 0.2;  // relative inset for tick path endpoints
+    private const double WfDropdownChevronSize = 4.5;  // half-width of the dropdown chevron arrow
 
     private static void AppendWireframeNode(StringBuilder sb, Node node, string kind, Theme theme)
     {
@@ -910,9 +912,9 @@ private static void AppendWfCheckbox(StringBuilder sb, Node node, Theme theme)
         {
             double cx = boxSize / 2;
             double cy = topY + boxSize / 2;
-            // Checkmark tick: two-segment path
-            double t = boxSize * 0.2;
-            sb.AppendLine($"""    <polyline points="{SvgRenderSupport.F(t)},{SvgRenderSupport.F(cy)} {SvgRenderSupport.F(cx * 0.75)},{SvgRenderSupport.F(topY + boxSize - t)} {SvgRenderSupport.F(boxSize - t)},{SvgRenderSupport.F(topY + t)}" fill="none" stroke="{SvgRenderSupport.Escape(WfCheckColor)}" stroke-width="1.5" stroke-linecap="round" stroke-linejoin="round"/>""");
+            // Checkmark tick: two-segment polyline (left-bottom valley → right-top peak)
+            double inset = boxSize * WfCheckmarkInsetRatio;
+            sb.AppendLine($"""    <polyline points="{SvgRenderSupport.F(inset)},{SvgRenderSupport.F(cy)} {SvgRenderSupport.F(cx * 0.75)},{SvgRenderSupport.F(topY + boxSize - inset)} {SvgRenderSupport.F(boxSize - inset)},{SvgRenderSupport.F(topY + inset)}" fill="none" stroke="{SvgRenderSupport.Escape(WfCheckColor)}" stroke-width="1.5" stroke-linecap="round" stroke-linejoin="round"/>""");
         }
 
         if (!string.IsNullOrWhiteSpace(node.Label.Text))
@@ -989,8 +991,7 @@ private static void AppendWfDropdown(StringBuilder sb, Node node, Theme theme)
         // Chevron icon on the right
         double chevX = node.Width - 18;
         double chevY = node.Height / 2;
-        double chevS = 4.5;
-        sb.AppendLine($"""    <polyline points="{SvgRenderSupport.F(chevX - chevS)},{SvgRenderSupport.F(chevY - chevS * 0.6)} {SvgRenderSupport.F(chevX)},{SvgRenderSupport.F(chevY + chevS * 0.6)} {SvgRenderSupport.F(chevX + chevS)},{SvgRenderSupport.F(chevY - chevS * 0.6)}" fill="none" stroke="{SvgRenderSupport.Escape(WfSubtleText)}" stroke-width="1.5" stroke-linecap="round" stroke-linejoin="round"/>""");
+        sb.AppendLine($"""    <polyline points="{SvgRenderSupport.F(chevX - WfDropdownChevronSize)},{SvgRenderSupport.F(chevY - WfDropdownChevronSize * 0.6)} {SvgRenderSupport.F(chevX)},{SvgRenderSupport.F(chevY + WfDropdownChevronSize * 0.6)} {SvgRenderSupport.F(chevX + WfDropdownChevronSize)},{SvgRenderSupport.F(chevY - WfDropdownChevronSize * 0.6)}" fill="none" stroke="{SvgRenderSupport.Escape(WfSubtleText)}" stroke-width="1.5" stroke-linecap="round" stroke-linejoin="round"/>""");
 
         // Vertical separator line before chevron
         sb.AppendLine($"""    <line x1="{SvgRenderSupport.F(node.Width - 28)}" y1="5" x2="{SvgRenderSupport.F(node.Width - 28)}" y2="{SvgRenderSupport.F(node.Height - 5)}" stroke="{SvgRenderSupport.Escape(WfInputBorder)}" stroke-width="0.8"/>""");
diff --git a/src/DiagramForge/Rendering/SvgRenderer.cs b/src/DiagramForge/Rendering/SvgRenderer.cs
@@ -128,7 +128,7 @@ int GetGroupDepth(Group group, HashSet<string>? visiting = null)
         foreach (var edge in diagram.Edges)
         {
             // Wireframe containment edges are layout-only; they must not produce visible SVG.
-            if (edge.Metadata.TryGetValue("wireframe:containment", out var wfc) && wfc is true)
+            if (edge.Metadata.TryGetValue("wireframe:containment", out var isWireframeContainment) && isWireframeContainment is true)
                 continue;
 
             if (!diagram.Nodes.TryGetValue(edge.SourceId, out var source)
@@ -148,7 +148,7 @@ int GetGroupDepth(Group group, HashSet<string>? visiting = null)
         foreach (var edge in diagram.Edges)
         {
             // Wireframe containment edges are layout-only; they must not produce visible SVG.
-            if (edge.Metadata.TryGetValue("wireframe:containment", out var wfc2) && wfc2 is true)
+            if (edge.Metadata.TryGetValue("wireframe:containment", out var isWireframeContainmentOverlay) && isWireframeContainmentOverlay is true)
                 continue;
 
             if (!diagram.Nodes.TryGetValue(edge.SourceId, out var source)
