Apply suggestions from code review

LinZhihao-723 · kirkrodrigues · web-flow · commit 404873412808 · 2024-10-11T13:15:43.000-04:00
Co-authored-by: kirkrodrigues &lt;2454684+kirkrodrigues@users.noreply.github.com&gt;
diff --git a/components/core/src/clp/ffi/KeyValuePairLogEvent.cpp b/components/core/src/clp/ffi/KeyValuePairLogEvent.cpp
@@ -219,7 +219,6 @@ auto validate_node_id_value_pairs(
         for (auto const& [node_id, value] : node_id_value_pairs) {
             auto const& node{schema_tree.get_node(node_id)};
             if (node.is_root()) {
-                // This node is the root
                 return std::errc::operation_not_permitted;
             }
 
@@ -241,7 +240,7 @@ auto validate_node_id_value_pairs(
                 return std::errc::operation_not_permitted;
             }
 
-            // Since we checked that the node must not be the root, we can query the underlying ID
+            // We checked that the node isn't the root above, so we can query the underlying ID
             // safely without a repeated check.
             auto const parent_node_id{node.get_parent_id_unsafe()};
             auto const key_name{node.get_key_name()};
@@ -298,10 +297,10 @@ auto get_schema_subtree_bitmap(
         // Iteratively mark the parents as true
         auto optional_parent_id{schema_tree.get_node(node_id).get_parent_id()};
         while (true) {
+            // Ideally, we'd use this if statement as the loop condition, but clang-tidy will
+            // complain about an unchecked `optional` access.
             if (false == optional_parent_id.has_value()) {
-                // Root has been reached.
-                // Ideally this if statement can be used as the loop condition. However, clang-tidy
-                // will complain about unchecked optional access.
+                // Reached the root
                 break;
             }
             auto const parent_id{optional_parent_id.value()};
diff --git a/components/core/src/clp/ffi/SchemaTree.hpp b/components/core/src/clp/ffi/SchemaTree.hpp
@@ -133,16 +133,15 @@ class SchemaTree {
         [[nodiscard]] auto is_root() const -> bool { return false == m_parent_id.has_value(); }
 
         /**
-         * @return the ID of the parent node in the schema tree if the node is not root.
-         * @return std::nullopt if the node is root.
+         * @return The ID of the parent node in the schema tree, if the node is not the root.
+         * @return std::nullopt if the node is the root.
          */
         [[nodiscard]] auto get_parent_id() const -> std::optional<id_t> { return m_parent_id; }
 
         /**
-         * Gets the parent ID without checking if it is std::nullopt.
+         * Gets the parent ID without checking if it's `std::nullopt`.
          * NOTE: This method should only be used if the caller has checked the node is not the root.
-         * `is_root()` must return false to call this method safely.
-         * @return the ID of the parent node in the schema tree.
+         * @return The ID of the parent node in the schema tree.
          */
         [[nodiscard]] auto get_parent_id_unsafe() const -> id_t {
             // NOLINTNEXTLINE(bugprone-unchecked-optional-access)
@@ -189,10 +188,9 @@ class SchemaTree {
 
         /**
          * Creates a root node.
-         * @param id The ID of the root node assigned by the schema tree.
          */
         [[nodiscard]] static auto create_root() -> Node {
-            return {cRootId, std::nullopt, "", Type::Obj};
+            return {cRootId, std::nullopt, {}, Type::Obj};
         }
 
         // Constructors
diff --git a/components/core/tests/test-ffi_SchemaTree.cpp b/components/core/tests/test-ffi_SchemaTree.cpp
@@ -25,8 +25,8 @@ namespace {
  * @param schema_tree
  * @param locator
  * @param expected_id
- * @return Whether the node exists with its ID matches the expected ID and its information matches
- * the ones specified in the locator.
+ * @return Whether an ID could be found for a node matching the locator, the ID matches the expected
+ * ID, the corresponding node is not the root, and it matches the locator.
  */
 [[nodiscard]] auto check_node(
         SchemaTree const& schema_tree,
@@ -50,17 +50,17 @@ auto check_node(
 ) -> bool {
     auto const node_id{schema_tree.try_get_node_id(locator)};
     if (false == node_id.has_value() || node_id.value() != expected_id) {
-        // Check node ID
+        // The node's ID doesn't match.
         return false;
     }
     auto const& node{schema_tree.get_node(expected_id)};
     if (node.is_root()) {
-        // The node after initialization must not be the root.
+        // Any nodes added after the tree was constructed must not be the root.
         return false;
     }
     auto const optional_parent_id{node.get_parent_id()};
     if (false == optional_parent_id.has_value()) {
-        // Non-root node must have a parent ID.
+        // Non-root nodes must have a parent ID.
         return false;
     }
     if (optional_parent_id.value() != locator.get_parent_id()
