Skip to content

Commit fedd331

Browse files
rafaelss95wKoza
rafaelss95
authored and
wKoza
committed
fix(rule): template-cyclomatic-complexity not reporting failures for '[ngForOf]' and '[ngIf]' (#612)
1 parent 5e34f41 commit fedd331

File tree

2 files changed

+114
-64
lines changed

2 files changed

+114
-64
lines changed

‎src/templateCyclomaticComplexityRule.ts‎

Lines changed: 40 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -1,35 +1,34 @@
1-
import * as Lint from 'tslint';
2-
import * as ts from 'typescript';
3-
import * as ast from '@angular/compiler';
1+
import { BoundDirectivePropertyAst } from '@angular/compiler';
42
import { sprintf } from 'sprintf-js';
5-
import { BasicTemplateAstVisitor } from './angular/templates/basicTemplateAstVisitor';
3+
import { IRuleMetadata, RuleFailure, Rules } from 'tslint/lib';
4+
import { SourceFile } from 'typescript/lib/typescript';
65
import { NgWalker } from './angular/ngWalker';
6+
import { BasicTemplateAstVisitor } from './angular/templates/basicTemplateAstVisitor';
77

8-
export class Rule extends Lint.Rules.AbstractRule {
9-
public static metadata: Lint.IRuleMetadata = {
10-
ruleName: 'template-cyclomatic-complexity',
11-
type: 'functionality',
8+
export class Rule extends Rules.AbstractRule {
9+
static readonly metadata: IRuleMetadata = {
1210
description:
1311
"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",
14-
rationale: 'Cyclomatic complexity over some threshold indicates that the logic should be moved outside the template.',
12+
optionExamples: ['true', '[true, 6]'],
1513
options: {
16-
type: 'array',
1714
items: {
1815
type: 'string'
1916
},
17+
maxLength: 2,
2018
minLength: 0,
21-
maxLength: 2
19+
type: 'array'
2220
},
23-
optionExamples: ['true', '[true, 6]'],
2421
optionsDescription: 'Determine the maximum number of the cyclomatic complexity.',
22+
rationale: 'Cyclomatic complexity over some threshold indicates that the logic should be moved outside the template.',
23+
ruleName: 'template-cyclomatic-complexity',
24+
type: 'functionality',
2525
typescriptOnly: true
2626
};
2727

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

32-
public apply(sourceFile: ts.SourceFile): Lint.RuleFailure[] {
31+
apply(sourceFile: SourceFile): RuleFailure[] {
3332
return this.applyWithWalker(
3433
new NgWalker(sourceFile, this.getOptions(), {
3534
templateVisitorCtrl: TemplateConditionalComplexityVisitor
@@ -38,33 +37,38 @@ export class Rule extends Lint.Rules.AbstractRule {
3837
}
3938
}
4039

40+
export const getFailureMessage = (maxComplexity = Rule.DEFAULT_MAX_COMPLEXITY): string => {
41+
return sprintf(Rule.FAILURE_STRING, maxComplexity);
42+
};
43+
4144
class TemplateConditionalComplexityVisitor extends BasicTemplateAstVisitor {
42-
complexity = 0;
45+
totalComplexity = 0;
4346

44-
visitDirectiveProperty(prop: ast.BoundDirectivePropertyAst, context: BasicTemplateAstVisitor): any {
45-
if (prop.sourceSpan) {
46-
const directive = (<any>prop.sourceSpan).toString();
47+
visitDirectiveProperty(prop: BoundDirectivePropertyAst, context: any): any {
48+
this.validateDirective(prop);
49+
super.visitDirectiveProperty(prop, context);
50+
}
4751

48-
if (
49-
directive.startsWith('*ngFor') ||
50-
directive.startsWith('*ngIf') ||
51-
directive.startsWith('*ngSwitchCase') ||
52-
directive.startsWith('*ngSwitchDefault')
53-
) {
54-
this.complexity++;
55-
}
52+
private validateDirective(prop: BoundDirectivePropertyAst): void {
53+
const pattern = /^ng(ForOf|If|Switch(Case|Default))$/;
54+
const { templateName } = prop;
55+
56+
if (pattern.test(templateName)) {
57+
this.totalComplexity++;
5658
}
5759

58-
const options = this.getOptions();
59-
const complexityMax: number = options.length ? options[0] : Rule.COMPLEXITY_MAX;
60+
const maxComplexity: number = this.getOptions()[0] || Rule.DEFAULT_MAX_COMPLEXITY;
6061

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

68-
super.visitDirectiveProperty(prop, context);
66+
const {
67+
sourceSpan: {
68+
end: { offset: endOffset },
69+
start: { offset: startOffset }
70+
}
71+
} = prop;
72+
this.addFailureFromStartToEnd(startOffset, endOffset, getFailureMessage(maxComplexity));
6973
}
7074
}

‎test/templateCyclomaticComplexityRule.spec.ts‎

Lines changed: 74 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -1,77 +1,123 @@
1-
import { assertSuccess, assertAnnotated } from './testHelper';
1+
import { getFailureMessage, Rule } from '../src/templateCyclomaticComplexityRule';
2+
import { assertAnnotated, assertSuccess } from './testHelper';
23

3-
describe('cyclomatic complexity', () => {
4-
describe('success', () => {
5-
it('should work with a lower level of complexity', () => {
6-
let source = `
4+
const {
5+
metadata: { ruleName }
6+
} = Rule;
7+
8+
describe(ruleName, () => {
9+
describe('failure', () => {
10+
it('should fail with a higher level of complexity', () => {
11+
const source = `
712
@Component({
813
template: \`
914
<div *ngIf="a === '1'">
10-
<li *ngFor="let person of persons; trackBy: trackByFn">
11-
{{ person.name }}
15+
<div *ngFor="let person of persons; trackBy: trackByFn">
16+
<div *ngIf="a === '1'">{{ person.name }}</div>
17+
1218
<div [ngSwitch]="person.emotion">
13-
<app-happy-hero *ngSwitchCase="'happy'" [hero]="currentHero"></app-happy-hero>
14-
<app-sad-hero *ngSwitchCase="'sad'" [hero]="currentHero"></app-sad-hero>
15-
<app-unknown-hero *ngSwitchDefault [hero]="currentHero"></app-unknown-hero>
19+
<app-happy-hero *ngSwitchCase="'happy'" [hero]="currentHero"></app-happy-hero>
20+
<app-sad-hero *ngSwitchCase="'sad'" [hero]="currentHero"></app-sad-hero>
21+
<app-confused-hero *ngSwitchCase="'confused'" [hero]="currentHero"></app-confused-hero>
22+
~~~~~~~~~~~~~~~~~~~~~~~~~~
23+
<app-unknown-hero *ngSwitchDefault [hero]="currentHero"></app-unknown-hero>
1624
</div>
17-
</li>
25+
</div>
1826
</div>
1927
\`
2028
})
2129
class Bar {}
2230
`;
23-
assertSuccess('template-cyclomatic-complexity', source);
31+
assertAnnotated({
32+
message: getFailureMessage(),
33+
ruleName,
34+
source
35+
});
2436
});
2537

26-
it('should work with a higher level of complexity', () => {
27-
let source = `
38+
it('should fail with a higher level of complexity using directives with ng-template', () => {
39+
const source = `
2840
@Component({
2941
template: \`
42+
<div [fakeDirective]="'test'"></div>
43+
44+
<ng-template ngFor let-person [ngForOf]="persons" let-i="index">
45+
{{ person.name }}
46+
</ng-template>
47+
48+
<ng-template [ngIf]="a === '1'">
49+
something here
50+
</ng-template>
51+
3052
<div *ngIf="a === '1'">
31-
<li *ngFor="let person of persons; trackBy: trackByFn">
53+
<div *ngFor="let person of persons; trackBy: trackByFn">
3254
<div *ngIf="a === '1'">{{ person.name }}</div>
55+
3356
<div [ngSwitch]="person.emotion">
3457
<app-happy-hero *ngSwitchCase="'happy'" [hero]="currentHero"></app-happy-hero>
3558
<app-sad-hero *ngSwitchCase="'sad'" [hero]="currentHero"></app-sad-hero>
59+
~~~~~~~~~~~~~~~~~~~~~
3660
<app-confused-hero *ngSwitchCase="'confused'" [hero]="currentHero"></app-confused-hero>
3761
<app-unknown-hero *ngSwitchDefault [hero]="currentHero"></app-unknown-hero>
3862
</div>
39-
</li>
63+
</div>
4064
</div>
4165
\`
4266
})
4367
class Bar {}
4468
`;
45-
assertSuccess('template-cyclomatic-complexity', source, [7]);
69+
assertAnnotated({
70+
message: getFailureMessage(6),
71+
options: [6],
72+
ruleName,
73+
source
74+
});
4675
});
4776
});
4877

49-
describe('failure', () => {
50-
it('should fail with a higher level of complexity', () => {
51-
let source = `
78+
describe('success', () => {
79+
it('should work with a lower level of complexity', () => {
80+
const source = `
81+
@Component({
82+
template: \`
83+
<div *ngIf="a === '1'">
84+
<div *ngFor="let person of persons; trackBy: trackByFn">
85+
{{ person.name }}
86+
<div [ngSwitch]="person.emotion">
87+
<app-happy-hero *ngSwitchCase="'happy'" [hero]="currentHero"></app-happy-hero>
88+
<app-sad-hero *ngSwitchCase="'sad'" [hero]="currentHero"></app-sad-hero>
89+
<app-unknown-hero *ngSwitchDefault [hero]="currentHero"></app-unknown-hero>
90+
</div>
91+
</div>
92+
</div>
93+
\`
94+
})
95+
class Bar {}
96+
`;
97+
assertSuccess(ruleName, source);
98+
});
99+
100+
it('should work with a higher level of complexity', () => {
101+
const source = `
52102
@Component({
53103
template: \`
54104
<div *ngIf="a === '1'">
55-
<li *ngFor="let person of persons; trackBy: trackByFn">
105+
<div *ngFor="let person of persons; trackBy: trackByFn">
56106
<div *ngIf="a === '1'">{{ person.name }}</div>
107+
57108
<div [ngSwitch]="person.emotion">
58109
<app-happy-hero *ngSwitchCase="'happy'" [hero]="currentHero"></app-happy-hero>
59110
<app-sad-hero *ngSwitchCase="'sad'" [hero]="currentHero"></app-sad-hero>
60111
<app-confused-hero *ngSwitchCase="'confused'" [hero]="currentHero"></app-confused-hero>
61-
~~~~~~~~~~~~~~~~~~~~~~~~~~
62112
<app-unknown-hero *ngSwitchDefault [hero]="currentHero"></app-unknown-hero>
63113
</div>
64-
</li>
114+
</div>
65115
</div>
66116
\`
67117
})
68118
class Bar {}
69119
`;
70-
assertAnnotated({
71-
ruleName: 'template-cyclomatic-complexity',
72-
message: "The cyclomatic complexity exceeded the defined limit (cost '5'). Your template should be refactored.",
73-
source
74-
});
120+
assertSuccess(ruleName, source, [7]);
75121
});
76122
});
77123
});

0 commit comments

Comments
 (0)