From 3a53a69379927d88ff6365b5b4b89055525f80c1 Mon Sep 17 00:00:00 2001
From: "Zheng, Lei" <realthunder.dev@gmail.com>
Date: Mon, 15 Jan 2024 21:22:57 -0600
Subject: [PATCH 1/3] App/Toponaming: Add Comparator for mapped elements

This is the original code from the Toponaming branch, modified slightly to update the
method name and correct some grammatical errors in the descriptive comment.

Co-authored-by: Chris Hennes <chennes@pioneerlibrarysystem.org>
---
 src/App/MappedElement.cpp | 102 ++++++++++++++++++++++++++++++++++++++
 src/App/MappedElement.h   |  16 ++++++
 2 files changed, 118 insertions(+)

diff --git a/src/App/MappedElement.cpp b/src/App/MappedElement.cpp
index 14eb6f99ba..5df937adfe 100644
--- a/src/App/MappedElement.cpp
+++ b/src/App/MappedElement.cpp
@@ -22,3 +22,105 @@
 
 #include "PreCompiled.h"
 #include "MappedElement.h"
+
+using namespace Data;
+
+bool ElementNameComparator::operator()(const MappedName &a, const MappedName &b) const {
+    size_t size = std::min(a.size(),b.size());
+    if(!size)
+        return a.size()<b.size();
+    size_t i=0;
+    if(b[0] == '#') {
+        if(a[0]!='#')
+            return true;
+        // If both string starts with '#', compare the following hex digits by
+        // its integer value.
+        int res = 0;
+        for(i=1;i<size;++i) {
+            unsigned char ac = (unsigned char)a[i];
+            unsigned char bc = (unsigned char)b[i];
+            if(std::isxdigit(bc)) {
+                if(!std::isxdigit(ac))
+                    return true;
+                if(res==0) {
+                    if(ac<bc)
+                        res = -1;
+                    else if(ac>bc)
+                        res = 1;
+                }
+            }else if(std::isxdigit(ac))
+                return false;
+            else
+                break;
+        }
+        if(res < 0)
+            return true;
+        else if(res > 0)
+            return false;
+
+        for (; i<size; ++i) {
+            char ac = a[i];
+            char bc = b[i];
+            if (ac < bc)
+                return true;
+            if (ac > bc)
+                return false;
+        }
+        return a.size()<b.size();
+    }
+    else if (a[0] == '#')
+        return false;
+
+    // If the string does not start with '#', compare the non-digits prefix
+    // using lexical order.
+    for(i=0;i<size;++i) {
+        unsigned char ac = (unsigned char)a[i];
+        unsigned char bc = (unsigned char)b[i];
+        if(!std::isdigit(bc)) {
+            if(std::isdigit(ac))
+                return true;
+            if(ac<bc)
+                return true;
+            if(ac>bc)
+                return false;
+        } else if(!std::isdigit(ac)) {
+            return false;
+        } else
+            break;
+    }
+
+    // Then compare the following digits part by integer value
+    int res = 0;
+    for(;i<size;++i) {
+        unsigned char ac = (unsigned char)a[i];
+        unsigned char bc = (unsigned char)b[i];
+        if(std::isdigit(bc)) {
+            if(!std::isdigit(ac))
+                return true;
+            if(res==0) {
+                if(ac<bc)
+                    res = -1;
+                else if(ac>bc)
+                    res = 1;
+            }
+        }else if(std::isdigit(ac))
+            return false;
+        else
+            break;
+    }
+    if(res < 0)
+        return true;
+    else if(res > 0)
+        return false;
+
+    // Finally, compare the remaining tail using lexical order
+    for (; i<size; ++i) {
+        char ac = a[i];
+        char bc = b[i];
+        if (ac < bc)
+            return true;
+        if (ac > bc)
+            return false;
+    }
+    return a.size()<b.size();
+}
\ No newline at end of file
diff --git a/src/App/MappedElement.h b/src/App/MappedElement.h
index 7f6ee68f01..9c37c3e8ba 100644
--- a/src/App/MappedElement.h
+++ b/src/App/MappedElement.h
@@ -98,6 +98,22 @@ struct AppExport MappedElement
     }
 };
 
+struct AppExport ElementNameComparator {
+    /** Comparison function to make topo name more stable
+     *
+     * The sorting decomposes the name into either of the following two forms
+     *      '#' + hex_digits + tail
+     *      non_digits + digits + tail
+     *
+     * The non-digits part is compared lexically, while the digits part is
+     * compared by its integer value.
+     *
+     * The reason for this is to prevent names with bigger digits (which usually means
+     * they come later in history) from coming earlier when sorting.
+     */
+    bool operator()(const MappedName &a, const MappedName &b) const;
+};
+
 }// namespace Data
 
 

From 210d4e925318e0622a607a1b6e7a618933cfd5a3 Mon Sep 17 00:00:00 2001
From: Chris Hennes <chennes@gmail.com>
Date: Mon, 15 Jan 2024 21:50:32 -0600
Subject: [PATCH 2/3] App/Toponaming: ElementNameComparator linter cleanup

Does not address cognitive complexity.
---
 src/App/MappedElement.cpp | 155 +++++++++++++++++++++++---------------
 src/App/MappedElement.h   |   2 +-
 2 files changed, 94 insertions(+), 63 deletions(-)

diff --git a/src/App/MappedElement.cpp b/src/App/MappedElement.cpp
index 5df937adfe..25e7abf7cc 100644
--- a/src/App/MappedElement.cpp
+++ b/src/App/MappedElement.cpp
@@ -25,102 +25,133 @@
 
 using namespace Data;
 
-bool ElementNameComparator::operator()(const MappedName &a, const MappedName &b) const {
-    size_t size = std::min(a.size(),b.size());
-    if(!size)
-        return a.size()<b.size();
-    size_t i=0;
-    if(b[0] == '#') {
-        if(a[0]!='#')
+bool ElementNameComparator::operator()(const MappedName& leftName,
+                                       const MappedName& rightName) const
+{
+    int size = static_cast<int>(std::min(leftName.size(), rightName.size()));
+    if (size == 0U) {
+        return leftName.size() < rightName.size();
+    }
+    int currentIndex = 0;
+    if (rightName[0] == '#') {
+        if (leftName[0] != '#') {
             return true;
+        }
         // If both string starts with '#', compare the following hex digits by
         // its integer value.
         int res = 0;
-        for(i=1;i<size;++i) {
-            unsigned char ac = (unsigned char)a[i];
-            unsigned char bc = (unsigned char)b[i];
-            if(std::isxdigit(bc)) {
-                if(!std::isxdigit(ac))
+        for (currentIndex = 1; currentIndex < size; ++currentIndex) {
+            auto ac = (unsigned char)leftName[currentIndex];
+            auto bc = (unsigned char)rightName[currentIndex];
+            if (std::isxdigit(bc) != 0) {
+                if (std::isxdigit(ac) == 0) {
                     return true;
-                if(res==0) {
-                    if(ac<bc)
-                        res = -1;
-                    else if(ac>bc)
-                        res = 1;
                 }
-            }else if(std::isxdigit(ac))
-                return false;
-            else
+                if (res == 0) {
+                    if (ac < bc) {
+                        res = -1;
+                    }
+                    else if (ac > bc) {
+                        res = 1;
+                    }
+                }
+            }
+            else if (std::isxdigit(ac) != 0) {
+                res = 1;
+            }
+            else {
                 break;
+            }
         }
-        if(res < 0)
+        if (res < 0) {
             return true;
-        else if(res > 0)
-            return false;
-
-        for (; i<size; ++i) {
-            char ac = a[i];
-            char bc = b[i];
-            if (ac < bc)
-                return true;
-            if (ac > bc)
-                return false;
         }
-        return a.size()<b.size();
+        if (res > 0) {
+            return false;
+        }
+
+        for (; currentIndex < size; ++currentIndex) {
+            char ac = leftName[currentIndex];
+            char bc = rightName[currentIndex];
+            if (ac < bc) {
+                return true;
+            }
+            if (ac > bc) {
+                return false;
+            }
+        }
+        return leftName.size() < rightName.size();
     }
-    else if (a[0] == '#')
+    if (leftName[0] == '#') {
         return false;
+    }
 
     // If the string does not start with '#', compare the non-digits prefix
     // using lexical order.
-    for(i=0;i<size;++i) {
-        unsigned char ac = (unsigned char)a[i];
-        unsigned char bc = (unsigned char)b[i];
-        if(!std::isdigit(bc)) {
-            if(std::isdigit(ac))
+    for (currentIndex = 0; currentIndex < size; ++currentIndex) {
+        auto ac = (unsigned char)leftName[currentIndex];
+        auto bc = (unsigned char)rightName[currentIndex];
+        if (std::isdigit(bc) == 0) {
+            if (std::isdigit(ac) != 0) {
                 return true;
-            if(ac<bc)
+            }
+            if (ac < bc) {
                 return true;
-            if(ac>bc)
+            }
+            if (ac > bc) {
                 return false;
-        } else if(!std::isdigit(ac)) {
+            }
+        }
+        else if (std::isdigit(ac) == 0) {
             return false;
-        } else
+        }
+        else {
             break;
+        }
     }
 
     // Then compare the following digits part by integer value
     int res = 0;
-    for(;i<size;++i) {
-        unsigned char ac = (unsigned char)a[i];
-        unsigned char bc = (unsigned char)b[i];
-        if(std::isdigit(bc)) {
-            if(!std::isdigit(ac))
+    for (; currentIndex < size; ++currentIndex) {
+        auto ac = (unsigned char)leftName[currentIndex];
+        auto bc = (unsigned char)rightName[currentIndex];
+        if (std::isdigit(bc) != 0) {
+            if (std::isdigit(ac) == 0) {
                 return true;
-            if(res==0) {
-                if(ac<bc)
-                    res = -1;
-                else if(ac>bc)
-                    res = 1;
             }
-        }else if(std::isdigit(ac))
+            if (res == 0) {
+                if (ac < bc) {
+                    res = -1;
+                }
+                else if (ac > bc) {
+                    res = 1;
+                }
+            }
+        }
+        else if (std::isdigit(ac) != 0) {
             return false;
-        else
+        }
+        else {
             break;
+        }
     }
-    if(res < 0)
+    if (res < 0) {
         return true;
-    else if(res > 0)
+    }
+    if (res > 0) {
         return false;
+    }
 
     // Finally, compare the remaining tail using lexical order
-    for (; i<size; ++i) {
-        char ac = a[i];
-        char bc = b[i];
-        if (ac < bc)
+    for (; currentIndex < size; ++currentIndex) {
+        char ac = leftName[currentIndex];
+        char bc = rightName[currentIndex];
+        if (ac < bc) {
             return true;
-        if (ac > bc)
+        }
+        if (ac > bc) {
             return false;
+        }
     }
-    return a.size()<b.size();
+    return leftName.size() < rightName.size();
 }
\ No newline at end of file
diff --git a/src/App/MappedElement.h b/src/App/MappedElement.h
index 9c37c3e8ba..798092176d 100644
--- a/src/App/MappedElement.h
+++ b/src/App/MappedElement.h
@@ -111,7 +111,7 @@ struct AppExport ElementNameComparator {
      * The reason for this is to prevent names with bigger digits (which usually means
      * they come later in history) from coming earlier when sorting.
      */
-    bool operator()(const MappedName &a, const MappedName &b) const;
+    bool operator()(const MappedName & leftName, const MappedName & rightName) const;
 };
 
 }// namespace Data

From 36451d1add8146a4e58f2ab50646dd618e1bcb03 Mon Sep 17 00:00:00 2001
From: Chris Hennes <chennes@gmail.com>
Date: Tue, 16 Jan 2024 19:05:40 -0600
Subject: [PATCH 3/3] Tests/Toponaming: Add tests for ElementNameComparator

This verifies the existing functionality, but does not alter it. Two tests are disabled because they represent cases that the current code does not handle correctly. They are edge cases that are not expected in real code.
---
 tests/src/App/MappedElement.cpp | 88 +++++++++++++++++++++++++++++++++
 1 file changed, 88 insertions(+)

diff --git a/tests/src/App/MappedElement.cpp b/tests/src/App/MappedElement.cpp
index 0ea03de6af..36588a8d92 100644
--- a/tests/src/App/MappedElement.cpp
+++ b/tests/src/App/MappedElement.cpp
@@ -127,3 +127,91 @@ TEST_F(MappedElementTest, lessThanOperator)
     EXPECT_FALSE(mappedElement2A < mappedElement1B);
     EXPECT_FALSE(mappedElement2B < mappedElement2BDuplicate);
 }
+
+TEST_F(MappedElementTest, comparatorBothAreZeroSize)
+{
+    // Arrange
+    Data::MappedName mappedName1 {""};
+    Data::MappedName mappedName2 {""};
+    auto comp = Data::ElementNameComparator();
+
+    // Act & Assert
+    EXPECT_FALSE(comp(mappedName1, mappedName2));
+}
+
+TEST_F(MappedElementTest, comparatorOneIsZeroSize)
+{
+    // Arrange
+    Data::MappedName mappedName1 {""};
+    Data::MappedName mappedName2 {"#12345"};
+    auto comp = Data::ElementNameComparator();
+
+    // Act & Assert
+    EXPECT_TRUE(comp(mappedName1, mappedName2));
+}
+
+TEST_F(MappedElementTest, comparatorBothStartWithHexDigitsThatDiffer)
+{
+    // Arrange
+    Data::MappedName mappedName1 {"#fed;B"};
+    Data::MappedName mappedName2 {"#abcdef;A"};
+    auto comp = Data::ElementNameComparator();
+
+    // Act & Assert
+    EXPECT_TRUE(comp(mappedName1, mappedName2));
+}
+
+TEST_F(MappedElementTest, comparatorBothStartWithTheSameHexDigits)
+{
+    // Arrange
+    Data::MappedName mappedName1 {"#12345;B"};
+    Data::MappedName mappedName2 {"#12345;A"};
+    auto comp = Data::ElementNameComparator();
+
+    // Act & Assert
+    EXPECT_FALSE(comp(mappedName1, mappedName2));
+}
+
+TEST_F(MappedElementTest, DISABLED_comparatorHexWithoutTerminatorIsBroken)
+{
+    // Arrange
+    Data::MappedName mappedName1 {"#fed"};
+    Data::MappedName mappedName2 {"#abcdef"};
+    auto comp = Data::ElementNameComparator();
+
+    // Act & Assert
+    EXPECT_FALSE(comp(mappedName1, mappedName2));
+}
+
+TEST_F(MappedElementTest, comparatorNoHexDigitsLexicalCompare)
+{
+    // Arrange
+    Data::MappedName mappedName1 {"A"};
+    Data::MappedName mappedName2 {"B"};
+    auto comp = Data::ElementNameComparator();
+
+    // Act & Assert
+    EXPECT_TRUE(comp(mappedName1, mappedName2));
+}
+
+TEST_F(MappedElementTest, comparatorNoHexDigitsSameStringNumericCompare)
+{
+    // Arrange
+    Data::MappedName mappedName1 {"Edge123456;"};
+    Data::MappedName mappedName2 {"Edge321;"};
+    auto comp = Data::ElementNameComparator();
+
+    // Act & Assert
+    EXPECT_FALSE(comp(mappedName1, mappedName2));
+}
+
+TEST_F(MappedElementTest, DISABLED_comparatorIntegerWithoutTerminatorIsBroken)
+{
+    // Arrange
+    Data::MappedName mappedName1 {"Edge123456"};
+    Data::MappedName mappedName2 {"Edge321"};
+    auto comp = Data::ElementNameComparator();
+
+    // Act & Assert
+    EXPECT_FALSE(comp(mappedName1, mappedName2));
+}