Address the SonarCloud findings from #57

Coder.Graph/AstNodeCatalog.cs

@@ -39,47 +39,59 @@ public sealed record AstNodeTemplate(string Category, string Label, Func<AstNode
 /// </remarks>
 public static class AstNodeCatalog
 {
+	/// <summary>The category a declaration is listed under.</summary>
+	private const string Declarations = "Declarations";
+
+	/// <summary>The category a statement is listed under.</summary>
+	private const string Statements = "Statements";
+
+	/// <summary>The category an expression is listed under.</summary>
+	private const string Expressions = "Expressions";
+
+	/// <summary>The category a literal is listed under.</summary>
+	private const string Literals = "Literals";
+
 	/// <summary>
 	/// Gets every node the palette offers, grouped and ordered for display.
 	/// </summary>
 	public static IReadOnlyList<AstNodeTemplate> Templates { get; } =
 	[
-		new("Declarations", "Class", () => new ClassDeclaration("NewClass")),
-		new("Declarations", "Function", () => new FunctionDeclaration("newFunction") { ReturnType = "void" }),
-		new("Declarations", "Parameter", () => new Parameter("value", "int")),
-		new("Declarations", "Variable", () => new VariableDeclaration("value", "int")),
-		new("Declarations", "Constant", () => new VariableDeclaration("VALUE", "int", Literal.Number(0)) { IsConstant = true }),
-		new("Declarations", "Entry point", () => new EntryPoint()),
-
-		new("Statements", "Return", () => new ReturnStatement()),
-		new("Statements", "Expression", () => new ExpressionStatement()),
+		new(Declarations, "Class", () => new ClassDeclaration("NewClass")),
+		new(Declarations, "Function", () => new FunctionDeclaration("newFunction") { ReturnType = "void" }),
+		new(Declarations, "Parameter", () => new Parameter("value", "int")),
+		new(Declarations, "Variable", () => new VariableDeclaration("value", "int")),
+		new(Declarations, "Constant", () => new VariableDeclaration("VALUE", "int", Literal.Number(0)) { IsConstant = true }),
+		new(Declarations, "Entry point", () => new EntryPoint()),
+
+		new(Statements, "Return", () => new ReturnStatement()),
+		new(Statements, "Expression", () => new ExpressionStatement()),
 		.. Enum.GetValues<AssignmentOperator>().Select(op => new AstNodeTemplate(
-			"Statements",
+			Statements,
 			Spell(op),
 			() => new AssignmentStatement(new VariableReference("target"), AstSchema.Unfilled(), op),
 			"Assignment")),
 
 		.. Enum.GetValues<BinaryOperator>().Select(op => new AstNodeTemplate(
-			"Expressions",
+			Expressions,
 			Spell(op),
 			() => new BinaryExpression(AstSchema.Unfilled(), op, AstSchema.Unfilled()),
 			"Binary")),
 		.. Enum.GetValues<UnaryOperator>().Select(op => new AstNodeTemplate(
-			"Expressions",
+			Expressions,
 			Spell(op),
 			() => new UnaryExpression(op, AstSchema.Unfilled()),
 			"Unary")),
-		new("Expressions", "Variable reference", () => new VariableReference("value")),
-		new("Expressions", "Call", () => new CallExpression("function")),
-		new("Expressions", "Conditional", () => new ConditionalExpression(
+		new(Expressions, "Variable reference", () => new VariableReference("value")),
+		new(Expressions, "Call", () => new CallExpression("function")),
+		new(Expressions, "Conditional", () => new ConditionalExpression(
 			AstSchema.Unfilled(),
 			AstSchema.Unfilled(),
 			AstSchema.Unfilled())),
 
-		new("Literals", "Text", () => Literal.Text("text")),
-		new("Literals", "Number", () => Literal.Number(0)),
-		new("Literals", "Decimal", () => Literal.DecimalValue(0)),
-		new("Literals", "Boolean", () => Literal.Bool(true)),
+		new(Literals, "Text", () => Literal.Text("text")),
+		new(Literals, "Number", () => Literal.Number(0)),
+		new(Literals, "Decimal", () => Literal.DecimalValue(0)),
+		new(Literals, "Boolean", () => Literal.Bool(true)),
 	];
 
 	/// <summary>

Coder.Graph/AstSchema.cs

@@ -25,10 +25,10 @@
 /// </remarks>
 public static class AstSchema
 {
 	private static readonly AstSlot ExpressionSlot = new("Expression", AstSlotCardinality.One, AstSlotKind.Expression);
 	private static readonly AstSlot LeftSlot = new("Left", AstSlotCardinality.One, AstSlotKind.Expression);
 	private static readonly AstSlot RightSlot = new("Right", AstSlotCardinality.One, AstSlotKind.Expression);
 	private static readonly AstSlot OperandSlot = new("Operand", AstSlotCardinality.One, AstSlotKind.Expression);
 	private static readonly AstSlot InitialValueSlot = new("InitialValue", AstSlotCardinality.One, AstSlotKind.Expression);
 	private static readonly AstSlot TargetSlot = new("Target", AstSlotCardinality.One, AstSlotKind.Expression);
 	private static readonly AstSlot ValueSlot = new("Value", AstSlotCardinality.One, AstSlotKind.Expression);
@@ -38,12 +38,27 @@
 	/// <summary>The name of the slot an expression's arguments sit in.</summary>
 	private const string ArgumentsSlotName = "Arguments";
 
+	/// <summary>The name of the slot a call's receiver sits in.</summary>
+	private const string ReceiverSlotName = "Receiver";
+
+	/// <summary>The name of the slot a conditional's condition sits in.</summary>
+	private const string ConditionSlotName = "Condition";
+
+	/// <summary>The name of the slot a conditional's chosen-when-true value sits in.</summary>
+	private const string WhenTrueSlotName = "WhenTrue";
+
+	/// <summary>The name of the slot a conditional's chosen-when-false value sits in.</summary>
+	private const string WhenFalseSlotName = "WhenFalse";
+
+	/// <summary>What a caption calls a declaration that has not been named yet.</summary>
+	private const string Unnamed = "<unnamed>";
+
 	private static readonly AstSlot ArgumentsSlot = new(ArgumentsSlotName, AstSlotCardinality.Many, AstSlotKind.Expression);
 	private static readonly AstSlot EnumMembersSlot = new("Members", AstSlotCardinality.Many, AstSlotKind.EnumMember);
-	private static readonly AstSlot ReceiverSlot = new("Receiver", AstSlotCardinality.One, AstSlotKind.Expression);
-	private static readonly AstSlot ConditionSlot = new("Condition", AstSlotCardinality.One, AstSlotKind.Expression);
-	private static readonly AstSlot WhenTrueSlot = new("WhenTrue", AstSlotCardinality.One, AstSlotKind.Expression);
-	private static readonly AstSlot WhenFalseSlot = new("WhenFalse", AstSlotCardinality.One, AstSlotKind.Expression);
+	private static readonly AstSlot ReceiverSlot = new(ReceiverSlotName, AstSlotCardinality.One, AstSlotKind.Expression);
+	private static readonly AstSlot ConditionSlot = new(ConditionSlotName, AstSlotCardinality.One, AstSlotKind.Expression);
+	private static readonly AstSlot WhenTrueSlot = new(WhenTrueSlotName, AstSlotCardinality.One, AstSlotKind.Expression);
+	private static readonly AstSlot WhenFalseSlot = new(WhenFalseSlotName, AstSlotCardinality.One, AstSlotKind.Expression);
 
 	/// <summary>
 	/// Lists the slots a node exposes, in the order the editor should draw them.
@@ -95,10 +110,10 @@
 			(MemberInitialiser initialiser, "Value") => initialiser.Value,
 			(AssignmentStatement assignment, "Target") => assignment.Target,
 			(AssignmentStatement assignment, "Value") => assignment.Value,
-			(CallExpression callExpr, "Receiver") => callExpr.Receiver,
-			(ConditionalExpression conditional, "Condition") => conditional.Condition,
-			(ConditionalExpression conditional, "WhenTrue") => conditional.WhenTrue,
-			(ConditionalExpression conditional, "WhenFalse") => conditional.WhenFalse,
+			(CallExpression callExpr, ReceiverSlotName) => callExpr.Receiver,
+			(ConditionalExpression conditional, ConditionSlotName) => conditional.Condition,
+			(ConditionalExpression conditional, WhenTrueSlotName) => conditional.WhenTrue,
+			(ConditionalExpression conditional, WhenFalseSlotName) => conditional.WhenFalse,
 			(ExpressionStatement statement, "Expression") => statement.Expression,
 			_ => null,
 		};
@@ -188,19 +203,19 @@
 				unary.Operand = operandExpr;
 				return true;
 
-			case (CallExpression callExpr, "Receiver") when child is Expression receiverExpr:
+			case (CallExpression callExpr, ReceiverSlotName) when child is Expression receiverExpr:
 				callExpr.Receiver = receiverExpr;
 				return true;
 
-			case (ConditionalExpression conditional, "Condition") when child is Expression conditionExpr:
+			case (ConditionalExpression conditional, ConditionSlotName) when child is Expression conditionExpr:
 				conditional.Condition = conditionExpr;
 				return true;
 
-			case (ConditionalExpression conditional, "WhenTrue") when child is Expression whenTrueExpr:
+			case (ConditionalExpression conditional, WhenTrueSlotName) when child is Expression whenTrueExpr:
 				conditional.WhenTrue = whenTrueExpr;
 				return true;
 
-			case (ConditionalExpression conditional, "WhenFalse") when child is Expression whenFalseExpr:
+			case (ConditionalExpression conditional, WhenFalseSlotName) when child is Expression whenFalseExpr:
 				conditional.WhenFalse = whenFalseExpr;
 				return true;
 
@@ -467,20 +482,20 @@
 
 			// A receiver is genuinely optional -- a call with none is a free function rather than an
 			// unfinished member call -- so detaching one clears it instead of leaving a placeholder.
-			case (CallExpression callExpr, "Receiver"):
+			case (CallExpression callExpr, ReceiverSlotName):
 				bool hadReceiver = callExpr.Receiver is not null;
 				callExpr.Receiver = null;
 				return hadReceiver;
 
-			case (ConditionalExpression conditional, "Condition"):
+			case (ConditionalExpression conditional, ConditionSlotName):
 				conditional.Condition = Unfilled();
 				return true;
 
-			case (ConditionalExpression conditional, "WhenTrue"):
+			case (ConditionalExpression conditional, WhenTrueSlotName):
 				conditional.WhenTrue = Unfilled();
 				return true;
 
-			case (ConditionalExpression conditional, "WhenFalse"):
+			case (ConditionalExpression conditional, WhenFalseSlotName):
 				conditional.WhenFalse = Unfilled();
 				return true;
 
@@ -589,12 +604,12 @@
 
 		return node switch
 		{
-			ClassDeclaration classDecl => $"class {classDecl.Name ?? "<unnamed>"}",
-			FunctionDeclaration function => $"function {function.Name ?? "<unnamed>"}",
+			ClassDeclaration classDecl => $"class {classDecl.Name ?? Unnamed}",
+			FunctionDeclaration function => $"function {function.Name ?? Unnamed}",
 			EntryPoint => "entry point",
-			Parameter parameter => $"param {parameter.Name ?? "<unnamed>"}",
+			Parameter parameter => $"param {parameter.Name ?? Unnamed}",
 			ReturnStatement => "return",
-			CallExpression callExpr => $"call {(callExpr.Callee.Length == 0 ? "<unnamed>" : callExpr.Callee)}",
+			CallExpression callExpr => $"call {(callExpr.Callee.Length == 0 ? Unnamed : callExpr.Callee)}",
 			ConditionalExpression => "conditional",
 			ExpressionStatement => "expression",
 			BinaryExpression binary => $"binary {SpellOrName(binary.Operator)}",

Coder.Test/Ast/CallExpressionTests.cs

@@ -26,7 +26,7 @@ public void CallExpression_ShouldCreateCorrectStructure()
 
 		Assert.AreEqual("sqrt", call.Callee);
 		Assert.IsNull(call.Receiver);
-		Assert.AreEqual(1, call.Arguments.Count);
+		Assert.HasCount(1, call.Arguments);
 	}
 
 	/// <summary>
@@ -75,7 +75,7 @@ public void CallExpression_Clone_IsDeep()
 		Assert.IsNotNull(clone.Receiver);
 		Assert.AreNotSame(original.Receiver, clone.Receiver);
 		Assert.AreEqual("point", ((VariableReference)clone.Receiver).Name);
-		Assert.AreEqual(2, clone.Arguments.Count);
+		Assert.HasCount(2, clone.Arguments);
 		Assert.AreNotSame(original.Arguments[0], clone.Arguments[0]);
 	}
 
@@ -210,7 +210,7 @@ public void CallExpression_RoundTripsThroughYaml()
 		Assert.AreEqual("void", roundTripped.ExpectedType);
 		Assert.IsInstanceOfType<VariableReference>(roundTripped.Receiver);
 		Assert.AreEqual("point", ((VariableReference)roundTripped.Receiver!).Name);
-		Assert.AreEqual(2, roundTripped.Arguments.Count);
+		Assert.HasCount(2, roundTripped.Arguments);
 		Assert.AreEqual("point.translate(dx, 3)", new CSharpGenerator().Generate(roundTripped));
 	}
 

Coder.Test/Ast/ExpressionStatementTests.cs

@@ -99,8 +99,8 @@ public void VoidCall_ReachesAFunctionBody()
 
 		string generated = new CSharpGenerator().Generate(function);
 
-		StringAssert.Contains(generated, "assert(ready);");
-		StringAssert.Contains(generated, "items.clear();");
+		StringAssert.Contains(generated, "assert(ready);", StringComparison.Ordinal);
+		StringAssert.Contains(generated, "items.clear();", StringComparison.Ordinal);
 	}
 
 	/// <summary>

Coder.Test/Graph/CallAndConditionalSlotsTests.cs

@@ -37,17 +37,17 @@ private static AstSlot Slot(AstNode node, string name) =>
 	[TestMethod]
 	public void SlotsOf_DescribesTheNewNodes()
 	{
-		CollectionAssert.AreEqual(
+		Assert.AreSequenceEqual(
 			CallSlots,
-			AstSchema.SlotsOf(new CallExpression("f")).Select(slot => slot.Name).ToArray());
+			AstSchema.SlotsOf(new CallExpression("f")).Select(slot => slot.Name));
 
-		CollectionAssert.AreEqual(
+		Assert.AreSequenceEqual(
 			ConditionalSlots,
-			AstSchema.SlotsOf(NewConditional()).Select(slot => slot.Name).ToArray());
+			AstSchema.SlotsOf(NewConditional()).Select(slot => slot.Name));
 
-		CollectionAssert.AreEqual(
+		Assert.AreSequenceEqual(
 			ExpressionStatementSlots,
-			AstSchema.SlotsOf(new ExpressionStatement()).Select(slot => slot.Name).ToArray());
+			AstSchema.SlotsOf(new ExpressionStatement()).Select(slot => slot.Name));
 	}
 
 	/// <summary>
@@ -59,7 +59,7 @@ public void ChildrenOf_ReportsAnAbsentReceiverAsEmpty()
 	{
 		CallExpression call = new("sqrt");
 
-		Assert.AreEqual(0, AstSchema.ChildrenOf(call, Slot(call, "Receiver")).Count);
+		Assert.IsEmpty(AstSchema.ChildrenOf(call, Slot(call, "Receiver")));
 	}
 
 	/// <summary>
@@ -76,7 +76,7 @@ public void TryAttach_FillsACall()
 		Assert.IsTrue(AstSchema.TryAttach(call, Slot(call, "Arguments"), new VariableReference("dy")));
 
 		Assert.AreSame(receiver, call.Receiver);
-		Assert.AreEqual(2, call.Arguments.Count);
+		Assert.HasCount(2, call.Arguments);
 		Assert.AreEqual("dx", ((VariableReference)call.Arguments[0]).Name);
 		Assert.AreEqual("dy", ((VariableReference)call.Arguments[1]).Name);
 	}
@@ -95,7 +95,7 @@ public void TryAttachAt_SwapsAnArgumentInPlace()
 
 		Assert.IsTrue(AstSchema.TryAttachAt(call, Slot(call, "Arguments"), 0, new VariableReference("z")));
 
-		Assert.AreEqual(2, call.Arguments.Count);
+		Assert.HasCount(2, call.Arguments);
 		Assert.AreEqual("z", ((VariableReference)call.Arguments[0]).Name);
 		Assert.AreEqual("b", ((VariableReference)call.Arguments[1]).Name);
 	}
@@ -132,7 +132,7 @@ public void TryDetachAt_RemovesAnArgument()
 
 		Assert.IsTrue(AstSchema.TryDetachAt(call, Slot(call, "Arguments"), 0));
 
-		Assert.AreEqual(1, call.Arguments.Count);
+		Assert.HasCount(1, call.Arguments);
 		Assert.AreEqual("b", ((VariableReference)call.Arguments[0]).Name);
 	}
 
@@ -187,7 +187,7 @@ public void AFunctionBody_TakesAnExpressionStatement()
 
 		Assert.IsTrue(AstSchema.TryAttach(function, body, new ExpressionStatement(new CallExpression("reset"))));
 
-		Assert.AreEqual(1, function.Body.Count);
+		Assert.HasCount(1, function.Body);
 	}
 
 	/// <summary>
@@ -239,27 +239,46 @@ public void AnEmptyCallee_IsRefused()
 	[TestMethod]
 	public void ThePalette_OffersTheNewNodes()
 	{
-		Assert.IsTrue(AstNodeCatalog.Templates.Any(template => template.Create() is CallExpression), "call");
-		Assert.IsTrue(AstNodeCatalog.Templates.Any(template => template.Create() is ConditionalExpression), "conditional");
-		Assert.IsTrue(AstNodeCatalog.Templates.Any(template => template.Create() is ExpressionStatement), "expression statement");
+		Type[] offered = [.. AstNodeCatalog.Templates.Select(template => template.Create().GetType())];
+
+		Assert.Contains(typeof(CallExpression), offered);
+		Assert.Contains(typeof(ConditionalExpression), offered);
+		Assert.Contains(typeof(ExpressionStatement), offered);
 	}
 
 	/// <summary>
 	/// Tests that what the palette creates is valid on its own, which is what lets a freshly created
 	/// node be connected rather than filled in first.
 	/// </summary>
+	/// <remarks>
+	/// "Valid on its own" means every slot can be read and every operand already in one is the
+	/// placeholder <see cref="AstSchema.Unfilled"/> produces, never a null the rest of the library
+	/// does not understand. A call's receiver is the one slot that starts empty rather than
+	/// placeholdered, because a call with no receiver is a free function rather than an unfinished
+	/// member call.
+	/// </remarks>
 	[TestMethod]
 	public void WhatThePaletteCreates_IsReadyToConnect()
 	{
-		foreach (AstNodeTemplate template in AstNodeCatalog.Templates
-			.Where(template => template.Create() is CallExpression or ConditionalExpression or ExpressionStatement))
+		AstNodeTemplate[] templates = [.. AstNodeCatalog.Templates
+			.Where(template => template.Create() is CallExpression or ConditionalExpression or ExpressionStatement)];
+
+		// Asserted rather than assumed: a filter that matched nothing would make the loop below pass
+		// without checking anything, which is exactly what this test exists to rule out.
+		Assert.HasCount(3, templates);
+
+		foreach (AstNodeTemplate template in templates)
 		{
 			AstNode node = template.Create();
 
 			foreach (AstSlot slot in AstSchema.SlotsOf(node))
 			{
-				// Reading a slot must not throw, whether or not anything is in it yet.
-				AstSchema.ChildrenOf(node, slot);
+				foreach (AstNode child in AstSchema.ChildrenOf(node, slot))
+				{
+					Assert.IsTrue(
+						AstSchema.IsUnfilled(child),
+						$"{AstSchema.Describe(node)}'s {slot.Name} should start unfilled, not as {AstSchema.Describe(child)}");
+				}
 			}
 		}
 	}