Refactor SplitList (#509)

axunonb · web-flow · commit 058f615314e6 · 2025-11-02T22:44:47.000+01:00
Class `Format`:
* Explicitly limit the max. number of splits to `int.MaxValue`
* `Format.Split(...)` now creates the `SplitList` cache, instead of the `SplitList` indexer

Class `SplitList`:
* Added new method `CreateSplitCache()`
* `SplitList` indexer throws if `SplitList` cache is not filled

Enhance unit test coverage for `SplitList`
diff --git a/src/SmartFormat.Tests/Core/ParserTests.cs b/src/SmartFormat.Tests/Core/ParserTests.cs
@@ -306,37 +306,6 @@ public void Test_Format_IndexOf()
         });
     }
 
-    [Test]
-    public void Test_Format_Split()
-    {
-        var parser = GetRegularParser();
-        var format = " a|aa {bbb: ccc dd|d {:|||} {eee} ff|f } gg|g ";
-        var parsedFormat = parser.ParseFormat(format);
-        var splits = parsedFormat.Split('|');
-
-        Assert.That(splits, Has.Count.EqualTo(3));
-        Assert.Multiple(() =>
-        {
-            Assert.That(splits[0].ToString(), Is.EqualTo(" a"));
-            Assert.That(splits[1].ToString(), Is.EqualTo("aa {bbb: ccc dd|d {:|||} {eee} ff|f } gg"));
-            Assert.That(splits[2].ToString(), Is.EqualTo("g "));
-        });
-
-        // Test nested formats:
-        var placeholder = (Placeholder) parsedFormat.Items[1];
-        parsedFormat = placeholder.Format!;
-        Assert.That(parsedFormat.ToString(), Is.EqualTo(" ccc dd|d {:|||} {eee} ff|f "));
-        splits = parsedFormat.Split('|');
-
-        Assert.That(splits, Has.Count.EqualTo(3));
-        Assert.Multiple(() =>
-        {
-            Assert.That(splits[0].ToString(), Is.EqualTo(" ccc dd"));
-            Assert.That(splits[1].ToString(), Is.EqualTo("d {:|||} {eee} ff"));
-            Assert.That(splits[2].ToString(), Is.EqualTo("f "));
-        });
-    }
-
     [TestCase("{0:name:}", "name", "", "")]
     [TestCase("{0:name()}", "name", "", "")]
     [TestCase("{0:name(1|2|3)}", "name", "1|2|3", "")]
diff --git a/src/SmartFormat.Tests/Core/Parsing/SplitListTests.cs b/src/SmartFormat.Tests/Core/Parsing/SplitListTests.cs
@@ -0,0 +1,131 @@
+﻿using System;
+using System.Collections.Generic;
+using NUnit.Framework;
+using SmartFormat.Core.Parsing;
+using SmartFormat.Core.Settings;
+
+namespace SmartFormat.Tests.Core.Parsing;
+
+[TestFixture]
+public class SplitListTests
+{
+    private static Parser GetRegularParser(SmartSettings? settings = null)
+    {
+        var parser = new Parser(settings ?? new SmartSettings
+            { StringFormatCompatibility = false, Parser = new ParserSettings { ErrorAction = ParseErrorAction.ThrowError } });
+        return parser;
+    }
+
+    [Test]
+    public void Test_Format_Split()
+    {
+        var parser = GetRegularParser();
+        const string format = " a|aa {bbb: ccc dd|d {:|||} {eee} ff|f } gg|g ";
+        var parsedFormat = parser.ParseFormat(format);
+        var splits = parsedFormat.Split('|');
+
+        Assert.That(splits, Has.Count.EqualTo(3));
+        Assert.Multiple(() =>
+        {
+            Assert.That(splits[0].ToString(), Is.EqualTo(" a"));
+            // Split character inside nested format (Placeholder) is ignored:
+            Assert.That(splits[1].ToString(), Is.EqualTo("aa {bbb: ccc dd|d {:|||} {eee} ff|f } gg"));
+            Assert.That(splits[2].ToString(), Is.EqualTo("g "));
+        });
+    }
+
+    [Test]
+    public void SplitList_Should_EnumerateFormats()
+    {
+        var parser = GetRegularParser();
+        // Pure literal with split characters
+        const string format = "a|b|c";
+        var parsedFormat = parser.ParseFormat(format);
+        var splits = parsedFormat.Split('|');
+
+        // foreach is using GetEnumerator()
+        var result1 = string.Empty;
+        foreach (var split in splits)
+        {
+            result1 += split.ToString();
+        }
+
+        // Use explicit interface for GetEnumerator
+        var result2 = string.Empty;
+        var enumerator = ((System.Collections.IEnumerable) splits).GetEnumerator();
+        using var enumeratorDisposable = (IDisposable) enumerator;
+        while (enumerator.MoveNext())
+        {
+            result2 += enumerator.Current!.ToString();
+        }
+
+        Assert.Multiple(() =>
+        {
+            Assert.That(result1, Is.EqualTo("abc"));
+            Assert.That(result2, Is.EqualTo("abc"));
+            Assert.That(splits.IsReadOnly, Is.EqualTo(true));
+        });
+    }
+
+    [Test]
+    public void CopyTo_ShouldCopyFormats()
+    {
+        var parser = GetRegularParser();
+        // Pure literal with split characters
+        const string format = "a|b|c";
+        var parsedFormat = parser.ParseFormat(format);
+        var splits = parsedFormat.Split('|');
+        var array = new Format[3];
+        splits.CopyTo(array, 0);
+
+        Assert.That(array, Is.EquivalentTo(splits));
+    }
+
+    [Test]
+    public void Indexer_ShouldThrow_IfCacheNotFilled()
+    {
+        var parser = GetRegularParser();
+        // Pure literal with split characters
+        const string format = "a|b";
+        var parsedFormat = parser.ParseFormat(format);
+        // Initialize SplitList directly without calling SplitList.CreateSplitCache()
+        var splits = new SplitList().Initialize(parsedFormat, [format.IndexOf('|')]);
+
+        Assert.That(() => _ = splits[0], Throws.Exception.InstanceOf<InvalidOperationException>());
+    }
+
+    [Test]
+    public void InvalidIndexCall_ShouldThrow()
+    {
+        var parser = GetRegularParser();
+        // Pure literal with split characters
+        const string format = "a|b|c";
+        var parsedFormat = parser.ParseFormat(format);
+        var splits = parsedFormat.Split('|');
+
+        Assert.That(() => _ = splits[int.MaxValue], Throws.Exception.InstanceOf<ArgumentOutOfRangeException>());
+    }
+
+    [Test]
+    public void UnsupportedMemberCalls_ShouldThrow()
+    {
+        // Track although SplitList is internal
+
+        var parser = GetRegularParser();
+        // Pure literal with split characters
+        const string format = "a|b|c";
+        var fmt = parser.ParseFormat(format);
+        var splits = fmt.Split('|');
+
+        Assert.Multiple(() =>
+        {
+            Assert.That(() => splits[0] = fmt, Throws.Exception.InstanceOf<NotSupportedException>());
+            Assert.That(() => splits.IndexOf(fmt), Throws.Exception.InstanceOf<NotSupportedException>());
+            Assert.That(() => splits.Insert(0, fmt), Throws.Exception.InstanceOf<NotSupportedException>());
+            Assert.That(() => splits.Add(fmt), Throws.Exception.InstanceOf<NotSupportedException>());
+            Assert.That(() => splits.Contains(fmt), Throws.Exception.InstanceOf<NotSupportedException>());
+            Assert.That(() => splits.Remove(fmt), Throws.Exception.InstanceOf<NotSupportedException>());
+            Assert.That(() => splits.RemoveAt(0), Throws.Exception.InstanceOf<NotSupportedException>());
+        });
+    }
+}
diff --git a/src/SmartFormat/Core/Parsing/Format.cs b/src/SmartFormat/Core/Parsing/Format.cs
@@ -287,15 +287,16 @@ private List<int> FindAll(char search, int maxCount)
 
     /// <summary>
     /// Splits the <see cref="Format"/> items by the given search character.
+    /// The number of splits is limited to <see cref="int.MaxValue"/>.
     /// </summary>
     /// <param name="search">The search character used to split.</param>
-    /// <returns></returns>
+    /// <returns>An <see cref="IList{T}"/> of <see cref="Format"/>s.</returns>
     public IList<Format> Split(char search)
     {
         if (_splitCache == null || _splitCacheChar != search)
         {
             _splitCacheChar = search;
-            _splitCache = Split(search, -1);
+            _splitCache = Split(search, int.MaxValue);
         }
 
         return _splitCache;
@@ -309,8 +310,11 @@ public IList<Format> Split(char search)
     /// <returns>An <see cref="IList{T}"/> of <see cref="Format"/>s.</returns>
     public IList<Format> Split(char search, int maxCount)
     {
+        maxCount = Math.Abs(maxCount);
         var splits = FindAll(search, maxCount);
+        // Initialize the SplitList and prefill the cache:
         var splitList = SplitListPool.Instance.Get().Initialize(this, splits);
+        splitList.CreateSplitCache();
 
         // Keep track of the split lists we create,
         // so that they can be returned to the object pool for later reuse.
diff --git a/src/SmartFormat/Core/Parsing/SplitList.cs b/src/SmartFormat/Core/Parsing/SplitList.cs
@@ -20,7 +20,7 @@ internal class SplitList : IList<Format>
 
     private Format _format = InitializationObject.Format;
     private List<int> _splits = InitializationObject.IntegerList;
-    private readonly List<Format?> _formatCache = new();
+    private readonly List<Format?> _splitFormatCache = new();
         
     /// <summary>
     /// CTOR for object pooling.
@@ -42,9 +42,10 @@ public SplitList Initialize(Format format, List<int> splits)
         _format = format;
         _splits = splits;
 
-        // Resize the cache to match
+        // Pre-size the cache to match
+        // If we have N splits, we have N+1 resulting formats (see Count property)
         for (var i = 0; i < Count; ++i)
-            _formatCache.Add(null);
+            _splitFormatCache.Add(null);
 
         return this;
     }
@@ -63,31 +64,44 @@ public Format this[int index]
 
             if (_splits.Count == 0) return _format;
 
-            // Return the cached version?
-            if (_formatCache[index] != null)
-                return _formatCache[index]!;
+            // The cache was initialized with nulls, but not yet filled
+            if (_splitFormatCache.Exists(c => c is null))
+                throw new InvalidOperationException("SplitList cache was not filled.");
 
+            // Return the result from the cache
+            // which was created in method Format.Split(...)
+            return _splitFormatCache[index]!;
+        }
+        set => throw new NotSupportedException();
+    }
+
+    internal void CreateSplitCache()
+    {
+        if (_splits.Count == 0) return;
+
+        // If we have N splits, we have N+1 resulting formats
+        for (var index = 0; index <= _splits.Count; index++)
+        {
+            Format f;
             if (index == 0)
             {
-                var f = _format.Substring(0, _splits[0]);
-                _formatCache[index] = f;
-                return f;
+                f = _format.Substring(0, _splits[0]);
+                _splitFormatCache[index] = f;
+                continue;
             }
 
             if (index == _splits.Count)
             {
-                var f = _format.Substring(_splits[index - 1] + 1);
-                _formatCache[index] = f;
-                return f;
+                f = _format.Substring(_splits[index - 1] + 1);
+                _splitFormatCache[index] = f;
+                continue;
             }
 
-            // Return the format between the splits
+            // The format between the splits
             var startIndex = _splits[index - 1] + 1;
-            var format = _format.Substring(startIndex, _splits[index] - startIndex);
-            _formatCache[index] = format;
-            return format;
+            f = _format.Substring(startIndex, _splits[index] - startIndex);
+            _splitFormatCache[index] = f;
         }
-        set => throw new NotSupportedException();
     }
 
     /// <summary>
@@ -101,11 +115,13 @@ public void Clear()
         _splits = InitializationObject.IntegerList;
 
         // Return the Formats we created to the pool
-        for (var i = 0; i < _formatCache.Count; i++)
-            if (_formatCache[i] != null)
-                FormatPool.Instance.Return(_formatCache[i]!);
-            
-        _formatCache.Clear();
+        for (var i = 0; i < _splitFormatCache.Count; i++)
+        {
+            if (_splitFormatCache[i] != null)
+                FormatPool.Instance.Return(_splitFormatCache[i]!);
+        }
+
+        _splitFormatCache.Clear();
     }
 
     ///<inheritdoc/>
@@ -121,6 +137,21 @@ public void CopyTo(Format[] array, int arrayIndex)
     ///<inheritdoc/>
     public bool IsReadOnly => true;
 
+    ///<inheritdoc/>
+    public IEnumerator<Format> GetEnumerator()
+    {
+        for (var i = 0; i < Count; i++)
+        {
+            yield return this[i];
+        }
+    }
+
+    ///<inheritdoc/>
+    IEnumerator IEnumerable.GetEnumerator()
+    {
+        return GetEnumerator();
+    }
+
     #endregion
 
     #region: NotSupported IList Interface :
@@ -173,21 +204,5 @@ public bool Remove(Format item)
         throw new NotSupportedException();
     }
 
-    /// <summary>
-    /// This method is not implemented.
-    /// </summary>
-    public IEnumerator<Format> GetEnumerator()
-    {
-        throw new NotSupportedException();
-    }
-
-    /// <summary>
-    /// This method is not implemented.
-    /// </summary>
-    IEnumerator IEnumerable.GetEnumerator()
-    {
-        throw new NotSupportedException();
-    }
-
     #endregion
 }
