From 759eee042fd56f431487e07563e8860f92acc7ab Mon Sep 17 00:00:00 2001 From: Anthony Date: Wed, 21 Oct 2020 20:57:21 +0100 Subject: [PATCH] Refactored according to PR feedback. --- .../Repositories/Implement/SimilarNodeName.cs | 98 +++++++------------ .../Repositories/SimilarNodeNameTests.cs | 15 +++ 2 files changed, 52 insertions(+), 61 deletions(-) diff --git a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/SimilarNodeName.cs b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/SimilarNodeName.cs index 73836ff69a..b613ea84aa 100644 --- a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/SimilarNodeName.cs +++ b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/SimilarNodeName.cs @@ -7,26 +7,20 @@ namespace Umbraco.Core.Persistence.Repositories.Implement { internal class SimilarNodeName { - const string SPACE_CHARACTER = " "; - public int Id { get; set; } public string Name { get; set; } - // gets a unique name public static string GetUniqueName(IEnumerable names, int nodeId, string nodeName) { - var cleanName = string.IsNullOrWhiteSpace(nodeName) ? SPACE_CHARACTER : nodeName; - var items = names .Where(x => x.Id != nodeId) // ignore same node .Select(x => x.Name); - var uniqueName = GetUniqueName(items, cleanName); + var uniqueName = GetUniqueName(items, nodeName); return uniqueName; } - public static string GetUniqueName(IEnumerable names, string name) { var model = new StructuredName(name); @@ -37,7 +31,7 @@ namespace Umbraco.Core.Persistence.Repositories.Implement // name is empty, and there are no other names with suffixes, so just return " (1)" if (model.IsEmptyName() && !items.Any()) { - model.SetSuffix(1); + model.Suffix = StructuredName.INITIAL_SUFFIX; return model.FullName; } @@ -49,76 +43,76 @@ namespace Umbraco.Core.Persistence.Repositories.Implement if (emptyNameSuffix > 0) { - model.SetSuffix(emptyNameSuffix); + model.Suffix = (uint?)emptyNameSuffix; return model.FullName; } } // no suffix - name without suffix does NOT exist, AND name with suffix does NOT exist - if (!model.HasSuffix() && !items.SimpleNameExists(model.Text) && !items.SuffixedNameExists()) + if (!model.Suffix.HasValue && !items.SimpleNameExists(model.Text) && !items.SuffixedNameExists()) { - model.SetNoSuffix(); + model.Suffix = StructuredName.NO_SUFFIX; return model.FullName; } // no suffix - name without suffix exists, however name with suffix does NOT exist - if (!model.HasSuffix() && items.SimpleNameExists(model.Text) && !items.SuffixedNameExists()) + if (!model.Suffix.HasValue && items.SimpleNameExists(model.Text) && !items.SuffixedNameExists()) { var firstSuffix = GetFirstSuffix(items); - model.SetSuffix(firstSuffix); + model.Suffix = (uint?)firstSuffix; return model.FullName; } // no suffix - name without suffix exists, AND name with suffix does exist - if (!model.HasSuffix() && items.SimpleNameExists(model.Text) && items.SuffixedNameExists()) + if (!model.Suffix.HasValue && items.SimpleNameExists(model.Text) && items.SuffixedNameExists()) { var nextSuffix = GetSuffixNumber(items); - model.SetSuffix(nextSuffix); + model.Suffix = (uint?)nextSuffix; return model.FullName; } // no suffix - name without suffix does NOT exist, however name with suffix exists - if (!model.HasSuffix() && !items.SimpleNameExists(model.Text) && items.SuffixedNameExists()) + if (!model.Suffix.HasValue && !items.SimpleNameExists(model.Text) && items.SuffixedNameExists()) { var nextSuffix = GetSuffixNumber(items); - model.SetSuffix(nextSuffix); + model.Suffix = (uint?)nextSuffix; return model.FullName; } // has suffix - name without suffix exists - if (model.HasSuffix() && items.SimpleNameExists(model.Text)) + if (model.Suffix.HasValue && items.SimpleNameExists(model.Text)) { var nextSuffix = GetSuffixNumber(items); - model.SetSuffix(nextSuffix); + model.Suffix = (uint?)nextSuffix; return model.FullName; } // has suffix - name without suffix does NOT exist // a case where the user added the suffix, so add a secondary suffix - if (model.HasSuffix() && !items.SimpleNameExists(model.Text)) + if (model.Suffix.HasValue && !items.SimpleNameExists(model.Text)) { - model.SetText(model.FullName); - model.SetNoSuffix(); + model.Text = model.FullName; + model.Suffix = StructuredName.NO_SUFFIX; // filter items based on full name with suffix items = items.Where(x => x.Text.InvariantStartsWith(model.FullName)); var secondarySuffix = GetFirstSuffix(items); - model.SetSuffix(secondarySuffix); + model.Suffix = (uint?)secondarySuffix; return model.FullName; } // has suffix - name without suffix also exists, therefore we simply increment - if (model.HasSuffix() && items.SimpleNameExists(model.Text)) + if (model.Suffix.HasValue && items.SimpleNameExists(model.Text)) { var nextSuffix = GetSuffixNumber(items); - model.SetSuffix(nextSuffix); + model.Suffix = (uint?)nextSuffix; return model.FullName; } @@ -130,7 +124,7 @@ namespace Umbraco.Core.Persistence.Repositories.Implement { const int suffixStart = 1; - if (!items.Any(x => x.Number == suffixStart)) + if (!items.Any(x => x.Suffix == suffixStart)) { // none of the suffixes are the same as suffixStart, so we can use suffixStart! return suffixStart; @@ -142,13 +136,13 @@ namespace Umbraco.Core.Persistence.Repositories.Implement private static int GetSuffixNumber(IEnumerable items) { int current = 1; - foreach (var item in items.OrderBy(x => x.Number)) + foreach (var item in items.OrderBy(x => x.Suffix)) { - if (item.Number == current) + if (item.Suffix == current) { current++; } - else if (item.Number > current) + else if (item.Suffix > current) { // do nothing - we found our number! // eg. when suffixes are 1 & 3, then this method is required to generate 2 @@ -161,16 +155,20 @@ namespace Umbraco.Core.Persistence.Repositories.Implement internal class StructuredName { - private const string Suffixed_Pattern = @"(.*)\s\((\d+)\)$"; - internal string Text { get; private set; } - internal int Number { get; private set; } + const string SPACE_CHARACTER = " "; + const string SUFFIXED_PATTERN = @"(.*) \(([1-9]\d*)\)$"; + internal const uint INITIAL_SUFFIX = 1; + internal static readonly uint? NO_SUFFIX = default; + + internal string Text { get; set; } + internal uint ? Suffix { get; set; } public string FullName { get { - string text = string.IsNullOrWhiteSpace(Text) ? Text.Trim() : Text; + string text = (Text == SPACE_CHARACTER) ? Text.Trim() : Text; - return Number > 0 ? $"{text} ({Number})" : text; + return Suffix > 0 ? $"{text} ({Suffix})" : text; } } @@ -179,48 +177,26 @@ namespace Umbraco.Core.Persistence.Repositories.Implement if (string.IsNullOrWhiteSpace(name)) { Text = SPACE_CHARACTER; - Number = 0; return; } - var rg = new Regex(Suffixed_Pattern); + var rg = new Regex(SUFFIXED_PATTERN); var matches = rg.Matches(name); if (matches.Count > 0) { var match = matches[0]; Text = match.Groups[1].Value; - int number; - Number = int.TryParse(match.Groups[2].Value, out number) ? number : 0; + int number = int.TryParse(match.Groups[2].Value, out number) ? number : 0; + Suffix = (uint?)(number); return; } else { Text = name; - Number = 0; } - } - - internal bool HasSuffix() - { - return Number > 0; - } - - internal void SetSuffix(int number) - { - Number = number; - } - - internal void SetNoSuffix() - { - Number = 0; - } - - internal void SetText(string text) - { - Text = text; - } + } internal bool IsEmptyName() { @@ -243,7 +219,7 @@ namespace Umbraco.Core.Persistence.Repositories.Implement internal static bool SuffixedNameExists(this IEnumerable items) { - return items.Any(x => x.HasSuffix()); + return items.Any(x => x.Suffix.HasValue); } } } diff --git a/src/Umbraco.Tests.Integration/Persistence/Repositories/SimilarNodeNameTests.cs b/src/Umbraco.Tests.Integration/Persistence/Repositories/SimilarNodeNameTests.cs index 14746b919c..688baad540 100644 --- a/src/Umbraco.Tests.Integration/Persistence/Repositories/SimilarNodeNameTests.cs +++ b/src/Umbraco.Tests.Integration/Persistence/Repositories/SimilarNodeNameTests.cs @@ -160,6 +160,21 @@ namespace Umbraco.Tests.Integration.Persistence.Repositories } + [TestCase("Test (0)", "Test (0) (1)")] + [TestCase("Test (-1)", "Test (-1) (1)")] + [TestCase("Test (1) (-1)", "Test (1) (-1) (1)")] + public void NonPositive_Suffix_Is_Ignored(string suffix, string expected) + { + var names = new[] + { + new SimilarNodeName { Id = 6, Name = suffix } + }; + var res = SimilarNodeName.GetUniqueName(names, 0, suffix); + + Assert.AreEqual(expected, res); + } + + /* Original Tests - Can be deleted, as new tests cover all cases */