fix(performance): refactoring inefficient duplication checking nested…

… for loops (#587) Signed-off-by: Stefan Blaginov <[email protected]>
accordproject · Jan 6, 2023 · 9e91f31 · 9e91f31
1 parent 20315ea
commit 9e91f31
Show file tree

Hide file tree

Showing 3 changed files with 68 additions and 39 deletions.
diff --git a/packages/concerto-core/lib/introspect/classdeclaration.js b/packages/concerto-core/lib/introspect/classdeclaration.js
@@ -212,12 +212,18 @@ class ClassDeclaration extends Decorated {
         super.validate();
 
         const declarations = this.getModelFile().getAllDeclarations();
-        const declarationNames = declarations.map(d => d.getFullyQualifiedName());
-        const uniqueNames = [...new Set(declarationNames)];
-
-        if (uniqueNames.length !== declarationNames.length) {
-            const duplicateElements = declarationNames.filter((item, index) => declarationNames.indexOf(item) !== index);
-            throw new IllegalModelException(`Duplicate class name ${duplicateElements[0]}`);
+        const declarationNames = declarations.map(
+            d => d.getFullyQualifiedName()
+        );
+        const uniqueNames = new Set(declarationNames);
+
+        if (uniqueNames.size !== declarations.length) {
+            const duplicateElements = declarationNames.filter(
+                (item, index) => declarationNames.indexOf(item) !== index
+            );
+            throw new IllegalModelException(
+                `Duplicate class name ${duplicateElements[0]}`
+            );
         }
 
         // if we have a super type make sure it exists
@@ -270,28 +276,38 @@ class ClassDeclaration extends Decorated {
                 }
             }
         }
-
         // we also have to check fields defined in super classes
         const properties = this.getProperties();
+        const propertyFieldNames = properties.map(
+            d => d.getName()
+        );
+        const uniquePropertyFieldNames = new Set(propertyFieldNames);
+
+        if (uniquePropertyFieldNames.size !== properties.length) {
+            const duplicateElements = propertyFieldNames
+                .filter(
+                    (item, index) => propertyFieldNames.indexOf(item) !== index
+                );
+            const formatter = Globalize('en').messageFormatter(
+                'classdeclaration-validate-duplicatefieldname'
+            );
+            throw new IllegalModelException(formatter({
+                'class': this.name,
+                'fieldName': duplicateElements[0]
+            }), this.modelFile, this.ast.location);
+        }
+
         for (let n = 0; n < properties.length; n++) {
             let field = properties[n];
 
-            // check we don't have a field with the same name
-            for (let i = n + 1; i < properties.length; i++) {
-                let otherField = properties[i];
-                if (field.getName() === otherField.getName()) {
-                    let formatter = Globalize('en').messageFormatter('classdeclaration-validate-duplicatefieldname');
-                    throw new IllegalModelException(formatter({
-                        'class': this.name,
-                        'fieldName': field.getName()
-                    }), this.modelFile, this.ast.location);
-                }
-            }
-
             // we now validate the field, however to ensure that
             // imports are resolved correctly we validate in the context
             // of the declared type of the field for non-primitives in a different namespace
-            if (field.isPrimitive() || this.isEnum() || field.getNamespace() === this.getNamespace()) {
+            if (
+                field.isPrimitive() ||
+                this.isEnum() ||
+                field.getNamespace() === this.getNamespace()
+            ) {
                 field.validate(this);
             } else {
                 const typeFqn = field.getFullyQualifiedTypeName();

diff --git a/packages/concerto-core/lib/introspect/decorated.js b/packages/concerto-core/lib/introspect/decorated.js
@@ -110,17 +110,28 @@ class Decorated {
      * @protected
      */
     validate(...args) {
-        for(let n=0; n < this.decorators.length; n++) {
-            let decorator = this.decorators[n];
-            decorator.validate();
+        if (this.decorators && this.decorators.length > 0) {
+            for(let n=0; n < this.decorators.length; n++) {
+                this.decorators[n].validate();
+            }
 
             // check we don't have this decorator twice
-            for(let i=n+1; i < this.decorators.length; i++) {
-                let otherDecorator = this.decorators[i];
-                if(decorator.getName() === otherDecorator.getName()) {
-                    let modelFile = this.getModelFile();
-                    throw new IllegalModelException(`Duplicate decorator ${decorator.getName()}`,modelFile, this.ast.location);
-                }
+            const decoratorNames = this.decorators.map(
+                d => d.getName()
+            );
+            const uniqueDecoratorNames = new Set(decoratorNames);
+
+            if (uniqueDecoratorNames.size !== this.decorators.length) {
+                const duplicateElements = decoratorNames
+                    .filter(
+                        (item, index) => decoratorNames.indexOf(item) !== index
+                    );
+                const modelFile = this.getModelFile();
+                throw new IllegalModelException(
+                    `Duplicate decorator ${duplicateElements[0]}`,
+                    modelFile,
+                    this.ast.location,
+                );
             }
         }
     }

diff --git a/packages/concerto-core/lib/introspect/scalardeclaration.js b/packages/concerto-core/lib/introspect/scalardeclaration.js
@@ -136,16 +136,18 @@ class ScalarDeclaration extends Decorated {
         super.validate();
 
         const declarations = this.getModelFile().getAllDeclarations();
-        for (let n = 0; n < declarations.length; n++) {
-            let declaration = declarations[n];
-
-            // check we don't have an asset with the same name
-            for (let i = n + 1; i < declarations.length; i++) {
-                let otherDeclaration = declarations[i];
-                if (declaration.getFullyQualifiedName() === otherDeclaration.getFullyQualifiedName()) {
-                    throw new IllegalModelException(`Duplicate class name ${declaration.getName()}`);
-                }
-            }
+        const declarationNames = declarations.map(
+            d => d.getFullyQualifiedName()
+        );
+        const uniqueNames = new Set(declarationNames);
+
+        if (uniqueNames.size !== declarations.length) {
+            const duplicateElements = declarationNames.filter(
+                (item, index) => declarationNames.indexOf(item) !== index
+            );
+            throw new IllegalModelException(
+                `Duplicate class name ${duplicateElements[0]}`
+            );
         }
     }