Skip to content

Commit

Permalink
fix(rule): template-cyclomatic-complexity not reporting failures for …
Browse files Browse the repository at this point in the history 
…'[ngForOf]' and '[ngIf]' (#612)
  • Loading branch information
@rafaelss95 @wKoza
rafaelss95 authored and wKoza committed May 7, 2018
1 parent 5e34f41 commit fedd331
Show file tree
Hide file tree
Showing 2 changed files with 114 additions and 64 deletions.
76 changes: 40 additions & 36 deletions src/templateCyclomaticComplexityRule.ts
View file
@@ -1,35 +1,34 @@
import * as Lint from 'tslint';
import * as ts from 'typescript';
import * as ast from '@angular/compiler';
import { BoundDirectivePropertyAst } from '@angular/compiler';
import { sprintf } from 'sprintf-js';
import { BasicTemplateAstVisitor } from './angular/templates/basicTemplateAstVisitor';
import { IRuleMetadata, RuleFailure, Rules } from 'tslint/lib';
import { SourceFile } from 'typescript/lib/typescript';
import { NgWalker } from './angular/ngWalker';
import { BasicTemplateAstVisitor } from './angular/templates/basicTemplateAstVisitor';

export class Rule extends Lint.Rules.AbstractRule {
public static metadata: Lint.IRuleMetadata = {
ruleName: 'template-cyclomatic-complexity',
type: 'functionality',
export class Rule extends Rules.AbstractRule {
static readonly metadata: IRuleMetadata = {
description:
"Checks cyclomatic complexity against a specified limit. It is a quantitative measure of the number of linearly independent paths through a program's source code",
rationale: 'Cyclomatic complexity over some threshold indicates that the logic should be moved outside the template.',
optionExamples: ['true', '[true, 6]'],
options: {
type: 'array',
items: {
type: 'string'
},
maxLength: 2,
minLength: 0,
maxLength: 2
type: 'array'
},
optionExamples: ['true', '[true, 6]'],
optionsDescription: 'Determine the maximum number of the cyclomatic complexity.',
rationale: 'Cyclomatic complexity over some threshold indicates that the logic should be moved outside the template.',
ruleName: 'template-cyclomatic-complexity',
type: 'functionality',
typescriptOnly: true
};

static COMPLEXITY_FAILURE_STRING = "The cyclomatic complexity exceeded the defined limit (cost '%s'). Your template should be refactored.";

static COMPLEXITY_MAX = 5;
static readonly FAILURE_STRING = "The cyclomatic complexity exceeded the defined limit (cost '%s'). Your template should be refactored.";
static readonly DEFAULT_MAX_COMPLEXITY = 5;

public apply(sourceFile: ts.SourceFile): Lint.RuleFailure[] {
apply(sourceFile: SourceFile): RuleFailure[] {
return this.applyWithWalker(
new NgWalker(sourceFile, this.getOptions(), {
templateVisitorCtrl: TemplateConditionalComplexityVisitor
Expand All @@ -38,33 +37,38 @@ export class Rule extends Lint.Rules.AbstractRule {
}
}

export const getFailureMessage = (maxComplexity = Rule.DEFAULT_MAX_COMPLEXITY): string => {
return sprintf(Rule.FAILURE_STRING, maxComplexity);
};

class TemplateConditionalComplexityVisitor extends BasicTemplateAstVisitor {
complexity = 0;
totalComplexity = 0;

visitDirectiveProperty(prop: ast.BoundDirectivePropertyAst, context: BasicTemplateAstVisitor): any {
if (prop.sourceSpan) {
const directive = (<any>prop.sourceSpan).toString();
visitDirectiveProperty(prop: BoundDirectivePropertyAst, context: any): any {
this.validateDirective(prop);
super.visitDirectiveProperty(prop, context);
}

if (
directive.startsWith('*ngFor') ||
directive.startsWith('*ngIf') ||
directive.startsWith('*ngSwitchCase') ||
directive.startsWith('*ngSwitchDefault')
) {
this.complexity++;
}
private validateDirective(prop: BoundDirectivePropertyAst): void {
const pattern = /^ng(ForOf|If|Switch(Case|Default))$/;
const { templateName } = prop;

if (pattern.test(templateName)) {
this.totalComplexity++;
}

const options = this.getOptions();
const complexityMax: number = options.length ? options[0] : Rule.COMPLEXITY_MAX;
const maxComplexity: number = this.getOptions()[0] || Rule.DEFAULT_MAX_COMPLEXITY;

if (this.complexity > complexityMax) {
const span = prop.sourceSpan;
let failureConfig: string[] = [String(complexityMax)];
failureConfig.unshift(Rule.COMPLEXITY_FAILURE_STRING);
this.addFailure(this.createFailure(span.start.offset, span.end.offset - span.start.offset, sprintf.apply(this, failureConfig)));
if (this.totalComplexity <= maxComplexity) {
return;
}

super.visitDirectiveProperty(prop, context);
const {
sourceSpan: {
end: { offset: endOffset },
start: { offset: startOffset }
}
} = prop;
this.addFailureFromStartToEnd(startOffset, endOffset, getFailureMessage(maxComplexity));
}
}
102 changes: 74 additions & 28 deletions test/templateCyclomaticComplexityRule.spec.ts
View file
@@ -1,77 +1,123 @@
import { assertSuccess, assertAnnotated } from './testHelper';
import { getFailureMessage, Rule } from '../src/templateCyclomaticComplexityRule';
import { assertAnnotated, assertSuccess } from './testHelper';

describe('cyclomatic complexity', () => {
describe('success', () => {
it('should work with a lower level of complexity', () => {
let source = `
const {
metadata: { ruleName }
} = Rule;

describe(ruleName, () => {
describe('failure', () => {
it('should fail with a higher level of complexity', () => {
const source = `
@Component({
template: \`
<div *ngIf="a === '1'">
<li *ngFor="let person of persons; trackBy: trackByFn">
{{ person.name }}
<div *ngFor="let person of persons; trackBy: trackByFn">
<div *ngIf="a === '1'">{{ person.name }}</div>
<div [ngSwitch]="person.emotion">
<app-happy-hero *ngSwitchCase="'happy'" [hero]="currentHero"></app-happy-hero>
<app-sad-hero *ngSwitchCase="'sad'" [hero]="currentHero"></app-sad-hero>
<app-unknown-hero *ngSwitchDefault [hero]="currentHero"></app-unknown-hero>
<app-happy-hero *ngSwitchCase="'happy'" [hero]="currentHero"></app-happy-hero>
<app-sad-hero *ngSwitchCase="'sad'" [hero]="currentHero"></app-sad-hero>
<app-confused-hero *ngSwitchCase="'confused'" [hero]="currentHero"></app-confused-hero>
~~~~~~~~~~~~~~~~~~~~~~~~~~
<app-unknown-hero *ngSwitchDefault [hero]="currentHero"></app-unknown-hero>
</div>
</li>
</div>
</div>
\`
})
class Bar {}
`;
assertSuccess('template-cyclomatic-complexity', source);
assertAnnotated({
message: getFailureMessage(),
ruleName,
source
});
});

it('should work with a higher level of complexity', () => {
let source = `
it('should fail with a higher level of complexity using directives with ng-template', () => {
const source = `
@Component({
template: \`
<div [fakeDirective]="'test'"></div>
<ng-template ngFor let-person [ngForOf]="persons" let-i="index">
{{ person.name }}
</ng-template>
<ng-template [ngIf]="a === '1'">
something here
</ng-template>
<div *ngIf="a === '1'">
<li *ngFor="let person of persons; trackBy: trackByFn">
<div *ngFor="let person of persons; trackBy: trackByFn">
<div *ngIf="a === '1'">{{ person.name }}</div>
<div [ngSwitch]="person.emotion">
<app-happy-hero *ngSwitchCase="'happy'" [hero]="currentHero"></app-happy-hero>
<app-sad-hero *ngSwitchCase="'sad'" [hero]="currentHero"></app-sad-hero>
~~~~~~~~~~~~~~~~~~~~~
<app-confused-hero *ngSwitchCase="'confused'" [hero]="currentHero"></app-confused-hero>
<app-unknown-hero *ngSwitchDefault [hero]="currentHero"></app-unknown-hero>
</div>
</li>
</div>
</div>
\`
})
class Bar {}
`;
assertSuccess('template-cyclomatic-complexity', source, [7]);
assertAnnotated({
message: getFailureMessage(6),
options: [6],
ruleName,
source
});
});
});

describe('failure', () => {
it('should fail with a higher level of complexity', () => {
let source = `
describe('success', () => {
it('should work with a lower level of complexity', () => {
const source = `
@Component({
template: \`
<div *ngIf="a === '1'">
<div *ngFor="let person of persons; trackBy: trackByFn">
{{ person.name }}
<div [ngSwitch]="person.emotion">
<app-happy-hero *ngSwitchCase="'happy'" [hero]="currentHero"></app-happy-hero>
<app-sad-hero *ngSwitchCase="'sad'" [hero]="currentHero"></app-sad-hero>
<app-unknown-hero *ngSwitchDefault [hero]="currentHero"></app-unknown-hero>
</div>
</div>
</div>
\`
})
class Bar {}
`;
assertSuccess(ruleName, source);
});

it('should work with a higher level of complexity', () => {
const source = `
@Component({
template: \`
<div *ngIf="a === '1'">
<li *ngFor="let person of persons; trackBy: trackByFn">
<div *ngFor="let person of persons; trackBy: trackByFn">
<div *ngIf="a === '1'">{{ person.name }}</div>
<div [ngSwitch]="person.emotion">
<app-happy-hero *ngSwitchCase="'happy'" [hero]="currentHero"></app-happy-hero>
<app-sad-hero *ngSwitchCase="'sad'" [hero]="currentHero"></app-sad-hero>
<app-confused-hero *ngSwitchCase="'confused'" [hero]="currentHero"></app-confused-hero>
~~~~~~~~~~~~~~~~~~~~~~~~~~
<app-unknown-hero *ngSwitchDefault [hero]="currentHero"></app-unknown-hero>
</div>
</li>
</div>
</div>
\`
})
class Bar {}
`;
assertAnnotated({
ruleName: 'template-cyclomatic-complexity',
message: "The cyclomatic complexity exceeded the defined limit (cost '5'). Your template should be refactored.",
source
});
assertSuccess(ruleName, source, [7]);
});
});
});

0 comments on commit fedd331

Please sign in to comment.