SonarQube - Is his really better code?

medic · Nov 23, 2024 · 17c4c04 · 17c4c04
1 parent 8e35f2d
commit 17c4c04
Show file tree

Hide file tree

Showing 3 changed files with 65 additions and 48 deletions.
diff --git a/src/lib/hierarchy-operations/index.js b/src/lib/hierarchy-operations/index.js
@@ -82,28 +82,32 @@ const HierarchyOperations = (db, options) => {
   }
 
   function reassignReports(reports, sourceId, destinationId, updatedReports) {
-    reports.forEach(report => {
+    function reassignReportWithSubject(report, subjectId) {
       let updated = false;
+      if (report[subjectId] === sourceId) {
+        report[subjectId] = destinationId;
+        updated = true;
+      }
+
+      if (report.fields[subjectId] === sourceId) {
+        report.fields[subjectId] = destinationId;
+        updated = true;
+      }
+
+      if (updated) {
+        const isAlreadyUpdated = !!updatedReports.find(updated => updated._id === report._id);
+        if (!isAlreadyUpdated) {
+          updatedReports.push(report);
+        }
+      }
+    }
+
+    for (const report of reports) {
       const subjectIds = ['patient_id', 'patient_uuid', 'place_id', 'place_uuid'];
       for (const subjectId of subjectIds) {
-        if (report[subjectId] === sourceId) {
-          report[subjectId] = destinationId;
-          updated = true;
-        }
-
-        if (report.fields[subjectId] === sourceId) {
-          report.fields[subjectId] = destinationId;
-          updated = true;
-        }
-
-        if (updated) {
-          const isAlreadyUpdated = !!updatedReports.find(updated => updated._id === report._id);
-          if (!isAlreadyUpdated) {
-            updatedReports.push(report);
-          }
-        }
+        reassignReportWithSubject(report, subjectId);
       }
-    });
+    }
   }
 
   function minifyLineageAndWriteToDisk(docs) {
@@ -136,23 +140,25 @@ const HierarchyOperations = (db, options) => {
   }
 
   function replaceLineageInContacts(descendantsAndSelf, replacementLineage, destinationId) {
-    return descendantsAndSelf.reduce((agg, doc) => {
+    const result = [];
+    for (const doc of descendantsAndSelf) {
       const docIsDestination = doc._id === destinationId;
       const startingFromIdInLineage = options.merge || !docIsDestination ? destinationId : undefined;
 
       // skip top-level because it will be deleted
       if (options.merge && docIsDestination) {
-        return agg;
+        continue;
       }
 
       const parentWasUpdated = lineageManipulation.replaceLineage(doc, 'parent', replacementLineage, startingFromIdInLineage, options);
       const contactWasUpdated = lineageManipulation.replaceLineage(doc, 'contact', replacementLineage, destinationId, options);
       const isUpdated = parentWasUpdated || contactWasUpdated;
       if (isUpdated) {
-        agg.push(doc);
+        result.push(doc);
       }
-      return agg;
-    }, []);
+    }
+
+    return result;
   }
 
   return { move };

diff --git a/src/lib/hierarchy-operations/lineage-constraints.js b/src/lib/hierarchy-operations/lineage-constraints.js
@@ -47,33 +47,37 @@ Enforce the list of allowed parents for each contact type
 Ensure we are not creating a circular hierarchy
 */
 const getMovingViolations = (mapTypeToAllowedParents, sourceDoc, destinationDoc) => {
-  const commonViolations = getCommonViolations(sourceDoc, destinationDoc);
-  if (commonViolations) {
-    return commonViolations;
-  }
+  function getContactTypeError() {
+    const sourceContactType = getContactType(sourceDoc);
+    const destinationType = getContactType(destinationDoc);
+    const rulesForContact = mapTypeToAllowedParents[sourceContactType];
+    if (!rulesForContact) {
+      return `cannot move contact with unknown type '${sourceContactType}'`;
+    }
 
-  if (!mapTypeToAllowedParents) {
-    return 'hierarchy constraints are undefined';
-  }
-
-  const sourceContactType = getContactType(sourceDoc);
-  const destinationType = getContactType(destinationDoc);
-  const rulesForContact = mapTypeToAllowedParents[sourceContactType];
-  if (!rulesForContact) {
-    return `cannot move contact with unknown type '${sourceContactType}'`;
+    const isPermittedMoveToRoot = !destinationDoc && rulesForContact.length === 0;
+    if (!isPermittedMoveToRoot && !rulesForContact.includes(destinationType)) {
+      return `contacts of type '${sourceContactType}' cannot have parent of type '${destinationType}'`;
+    }
   }
 
-  const isPermittedMoveToRoot = !destinationDoc && rulesForContact.length === 0;
-  if (!isPermittedMoveToRoot && !rulesForContact.includes(destinationType)) {
-    return `contacts of type '${sourceContactType}' cannot have parent of type '${destinationType}'`;
+  function findCircularHierarchyErrors() {
+    if (destinationDoc && sourceDoc._id) {
+      const parentAncestry = [destinationDoc._id, ...lineageManipulation.pluckIdsFromLineage(destinationDoc.parent)];
+      if (parentAncestry.includes(sourceDoc._id)) {
+        return `Circular hierarchy: Cannot set parent of contact '${sourceDoc._id}' as it would create a circular hierarchy.`;
+      }
+    }
   }
 
-  if (destinationDoc && sourceDoc._id) {
-    const parentAncestry = [destinationDoc._id, ...lineageManipulation.pluckIdsFromLineage(destinationDoc.parent)];
-    if (parentAncestry.includes(sourceDoc._id)) {
-      return `Circular hierarchy: Cannot set parent of contact '${sourceDoc._id}' as it would create a circular hierarchy.`;
-    }
+  if (!mapTypeToAllowedParents) {
+    return 'hierarchy constraints are undefined';
   }
+
+  const commonViolations = getCommonViolations(sourceDoc, destinationDoc);
+  const contactTypeError = getContactTypeError();
+  const circularHierarchyError = findCircularHierarchyErrors();
+  return commonViolations || contactTypeError || circularHierarchyError;
 };
 
 const getCommonViolations = (sourceDoc, destinationDoc) => {

diff --git a/src/lib/hierarchy-operations/lineage-manipulation.js b/src/lib/hierarchy-operations/lineage-manipulation.js
@@ -15,7 +15,7 @@ function replaceLineage(doc, lineageAttributeName, replaceWith, startingFromIdIn
     return replaceWithinLineage(doc, lineageAttributeName, replaceWith);
   }
 
-  const getInitialState = () => {
+  function getInitialState() {
     if (options.merge) {
       return {
         element: doc,
@@ -27,10 +27,9 @@ function replaceLineage(doc, lineageAttributeName, replaceWith, startingFromIdIn
       element: doc[lineageAttributeName],
       attributeName: 'parent',
     };
-  };
+  }
 
-  const state = getInitialState();
-  while (state.element) {
+  function traverseOne() {
     const compare = options.merge ? state.element[state.attributeName] : state.element;
     if (compare?._id === startingFromIdInLineage) {
       return replaceWithinLineage(state.element, state.attributeName, replaceWith);
@@ -40,6 +39,14 @@ function replaceLineage(doc, lineageAttributeName, replaceWith, startingFromIdIn
     state.attributeName = 'parent';
   }
 
+  const state = getInitialState();
+  while (state.element) {
+    const result = traverseOne();
+    if (result) {
+      return result;
+    }
+  }
+
   return false;
 }