From feac38df81c09cea5957a4770b5eb6779a527366 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 27 Sep 2026 20:26:22 +0000 Subject: [PATCH] Enumerate a CompositeCommand's commands once [patch] The constructor enumerated its commands argument three times: once for the affected items, once for the total size and once for the command list. A single-pass sequence was empty by the third pass and threw "must contain at least one command", and a Select factory ran three times, so the metadata could describe different command instances from the ones that execute. The public constructor now materializes the sequence once and chains to a private constructor that computes the metadata from that list. A null element is rejected with ArgumentException instead of failing later. Fixes ktsu-dev/UndoRedo#108 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01M9aefrfAYJpanQVrUuFpJh --- UndoRedo.Test/CompositeCommandTests.cs | 74 ++++++++++++++++++++++++++ UndoRedo/CompositeCommand.cs | 39 ++++++++------ 2 files changed, 97 insertions(+), 16 deletions(-) diff --git a/UndoRedo.Test/CompositeCommandTests.cs b/UndoRedo.Test/CompositeCommandTests.cs index be8769d..90c2c28 100644 --- a/UndoRedo.Test/CompositeCommandTests.cs +++ b/UndoRedo.Test/CompositeCommandTests.cs @@ -269,4 +269,78 @@ public void CompositeCommand_LargeNumberOfCommands_PerformanceTest() Assert.IsEmpty(values); Assert.IsLessThan(1000L, stopwatch.ElapsedMilliseconds, "Undo should be fast"); } + + [TestMethod] + public void CompositeCommand_SinglePassSequence_KeepsEveryCommandAndItsMetadata() + { + // Arrange + List values = []; + + // Act + CompositeCommand composite = new("Single pass", SinglePass(values)); + composite.Execute(); + + // Assert + Assert.HasCount(2, composite.Commands); + CollectionAssert.AreEqual(new List { "A", "B" }, values); + CollectionAssert.AreEqual(new List { "item-A", "item-B" }, composite.Metadata.AffectedItems.ToList()); + Assert.AreEqual(5, composite.Metadata.Size); + } + + [TestMethod] + public void CompositeCommand_LazyFactory_RunsOncePerCommand() + { + // Arrange + int created = 0; + IEnumerable commands = Enumerable.Range(0, 3).Select(i => + { + created++; + return (ICommand)new DelegateCommand($"Command {i}", () => { }, () => { }, affectedItems: [$"item-{i}"], size: 1); + }); + + // Act + CompositeCommand composite = new("Lazy", commands); + + // Assert + Assert.AreEqual(3, created); + Assert.HasCount(3, composite.Metadata.AffectedItems); + Assert.AreEqual(3, composite.Metadata.Size); + } + + [TestMethod] + public void CompositeCommand_NullElement_ThrowsArgumentException() + { + // Arrange + ICommand[] commands = [new DelegateCommand("Ok", () => { }, () => { }), null!]; + + // Act & Assert + Assert.ThrowsExactly(() => new CompositeCommand("Null element", commands)); + } + + private static SinglePassSequence SinglePass(List values) => new( + [ + new DelegateCommand("Add A", () => values.Add("A"), () => values.RemoveAt(values.Count - 1), affectedItems: ["item-A"], size: 2), + new DelegateCommand("Add B", () => values.Add("B"), () => values.RemoveAt(values.Count - 1), affectedItems: ["item-B"], size: 3), + ]); + + /// + /// A sequence that yields its items only on the first enumeration, like a stream or a database cursor. + /// + private sealed class SinglePassSequence(IEnumerable items) : IEnumerable + { + private bool enumerated; + + public IEnumerator GetEnumerator() + { + if (enumerated) + { + return Enumerable.Empty().GetEnumerator(); + } + + enumerated = true; + return items.GetEnumerator(); + } + + System.Collections.IEnumerator System.Collections.IEnumerable.GetEnumerator() => GetEnumerator(); + } } diff --git a/UndoRedo/CompositeCommand.cs b/UndoRedo/CompositeCommand.cs index 821e14f..4eaeff1 100644 --- a/UndoRedo/CompositeCommand.cs +++ b/UndoRedo/CompositeCommand.cs @@ -24,19 +24,21 @@ public sealed class CompositeCommand : BaseCommand /// Commands to execute as a group /// Optional navigation context public CompositeCommand(string description, IEnumerable commands, string? navigationContext = null) + : this(description, Materialize(commands), navigationContext) + { + } + + // Takes the one materialized list, so the caller's sequence is enumerated exactly once and the + // metadata is computed from the same commands that Execute and Undo run. + private CompositeCommand(string description, List commands, string? navigationContext) : base( ChangeType.Composite, - GetAffectedItems(commands), + [.. commands.SelectMany(c => c.Metadata.AffectedItems).Distinct()], navigationContext, - GetTotalSize(commands)) + commands.Sum(c => c.Metadata.Size)) { Description = description; - _commands = [.. commands]; - - if (_commands.Count == 0) - { - throw new ArgumentException("Composite command must contain at least one command", nameof(commands)); - } + _commands = commands; } /// @@ -114,15 +116,20 @@ public override void Undo() /// public override ICommand MergeWith(ICommand other) => throw new NotSupportedException("Composite commands cannot be merged"); - private static IReadOnlyList GetAffectedItems(IEnumerable commands) + private static List Materialize(IEnumerable commands) { - List commandList = [.. commands]; - return [.. commandList.SelectMany(c => c.Metadata.AffectedItems).Distinct()]; - } + List commandList = [.. Ensure.NotNull(commands)]; - private static int GetTotalSize(IEnumerable commands) - { - List commandList = [.. commands]; - return commandList.Sum(c => c.Metadata.Size); + if (commandList.Count == 0) + { + throw new ArgumentException("Composite command must contain at least one command", nameof(commands)); + } + + if (commandList.Contains(null!)) + { + throw new ArgumentException("Composite command cannot contain a null command", nameof(commands)); + } + + return commandList; } }