From 9e91f310bee32c89f85c2695cc854bf974bcedcd Mon Sep 17 00:00:00 2001 From: Stefan Blaginov Date: Fri, 6 Jan 2023 17:26:27 +0200 Subject: [PATCH] fix(performance): refactoring inefficient duplication checking nested for loops (#587) Signed-off-by: Stefan Blaginov --- .../lib/introspect/classdeclaration.js | 56 ++++++++++++------- .../concerto-core/lib/introspect/decorated.js | 29 +++++++--- .../lib/introspect/scalardeclaration.js | 22 ++++---- 3 files changed, 68 insertions(+), 39 deletions(-) diff --git a/packages/concerto-core/lib/introspect/classdeclaration.js b/packages/concerto-core/lib/introspect/classdeclaration.js index 03131264ed..c04d660d88 100644 --- 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 index 0bbf5ee07f..a91c3fd9c0 100644 --- 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 index b788e4d532..c422147583 100644 --- 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]}` + ); } }