Skip to content

Commit b40b35c

Browse files
devilldevill
authored
Merge pull request #77 from fletchgqc/issue-73/variable-name-validator-improvements
Fix issue #73: Complete variable name validator improvements with systematic refactoring
2 parents 0a30009 + 199a4b9 commit b40b35c

7 files changed

Lines changed: 164 additions & 211 deletions

File tree

‎.quality-baseline.json‎

Lines changed: 32 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -381,12 +381,6 @@
381381
"singleUseVariable"
382382
]
383383
},
384-
"src/core/services/variable-name-validator.ts": {
385-
"lastCommitId": "4d7170d18e23cea4dc86b7c500ac66e148a95683",
386-
"violations": [
387-
"singleUseVariable"
388-
]
389-
},
390384
"src/core/services/variable-reference-request.ts": {
391385
"lastCommitId": "5add9ade67733f16f53716881aab85e467d3c68f",
392386
"violations": [
@@ -609,13 +603,6 @@
609603
"singleUseVariable"
610604
]
611605
},
612-
"tests/unit/services/variable-name-validator.test.ts": {
613-
"lastCommitId": "0f4e7736995c00491695acae2238fe55da1bd0a0",
614-
"violations": [
615-
"fileSize",
616-
"singleUseVariable"
617-
]
618-
},
619606
"tests/utils/file-operations.ts": {
620607
"lastCommitId": "89ea54e72e552715acec38f728fc0face807d2ad",
621608
"violations": [
@@ -631,24 +618,6 @@
631618
"singleUseVariable"
632619
]
633620
},
634-
"tests/unit/cli-location-handling.test.ts": {
635-
"lastCommitId": "1690de88db858021fcf493912253e06dab9dd7b4",
636-
"violations": [
637-
"singleUseVariable"
638-
]
639-
},
640-
"tests/unit/file-validator.test.ts": {
641-
"lastCommitId": "5add9ade67733f16f53716881aab85e467d3c68f",
642-
"violations": [
643-
"singleUseVariable"
644-
]
645-
},
646-
"tests/unit/usage-tracker.test.ts": {
647-
"lastCommitId": "608b51cb41bc489a35141052b3269596700e71f3",
648-
"violations": [
649-
"singleUseVariable"
650-
]
651-
},
652621
"tests/utils/fixture-location.ts": {
653622
"lastCommitId": "c419bc00c74e093452bf8eb2aa0e61e34598225d",
654623
"violations": [
@@ -709,8 +678,20 @@
709678
"singleUseVariable"
710679
]
711680
},
712-
"tests/integration/fixture.test.ts": {
713-
"lastCommitId": "f7d3433d1b8e57e08257e50af05bf009f175e2d0",
681+
"tests/unit/cli-location-handling.test.ts": {
682+
"lastCommitId": "1690de88db858021fcf493912253e06dab9dd7b4",
683+
"violations": [
684+
"singleUseVariable"
685+
]
686+
},
687+
"tests/unit/file-validator.test.ts": {
688+
"lastCommitId": "5add9ade67733f16f53716881aab85e467d3c68f",
689+
"violations": [
690+
"singleUseVariable"
691+
]
692+
},
693+
"tests/unit/usage-tracker.test.ts": {
694+
"lastCommitId": "608b51cb41bc489a35141052b3269596700e71f3",
714695
"violations": [
715696
"singleUseVariable"
716697
]
@@ -733,6 +714,24 @@
733714
"singleUseVariable"
734715
]
735716
},
717+
"tests/integration/fixture.test.ts": {
718+
"lastCommitId": "f7d3433d1b8e57e08257e50af05bf009f175e2d0",
719+
"violations": [
720+
"singleUseVariable"
721+
]
722+
},
723+
"tests/utils/parsers/location-parser.ts": {
724+
"lastCommitId": "d9034ceebf77dd46366b48298985149e2e7922bb",
725+
"violations": [
726+
"singleUseVariable"
727+
]
728+
},
729+
"tests/utils/parsers/meta-parser.ts": {
730+
"lastCommitId": "fb39b25c30bad5bbfce825da839abba185c7a1d4",
731+
"violations": [
732+
"singleUseVariable"
733+
]
734+
},
736735
"tests/unit/core/location-parser.test.ts": {
737736
"lastCommitId": "d9034ceebf77dd46366b48298985149e2e7922bb",
738737
"violations": [
@@ -913,18 +912,6 @@
913912
"singleUseVariable"
914913
]
915914
},
916-
"tests/utils/parsers/location-parser.ts": {
917-
"lastCommitId": "d9034ceebf77dd46366b48298985149e2e7922bb",
918-
"violations": [
919-
"singleUseVariable"
920-
]
921-
},
922-
"tests/utils/parsers/meta-parser.ts": {
923-
"lastCommitId": "fb39b25c30bad5bbfce825da839abba185c7a1d4",
924-
"violations": [
925-
"singleUseVariable"
926-
]
927-
},
928915
"tests/unit/locators/services/node-assignment-analyzer.test.ts": {
929916
"lastCommitId": "e37f849566f5affaa17254ceaebb6b99b12da3d9",
930917
"violations": [

‎guides/REFACTORING_STYLE.md‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -130,4 +130,10 @@ const lineCount = this.getLineCount(func);
130130
### Naming Conventions
131131
- **Intention-revealing names** that describe the business purpose
132132
- **Avoid technical jargon** in favor of domain language
133-
- **Use verbs for methods, nouns for classes and properties**
133+
- **Use verbs for methods, nouns for classes and properties**
134+
135+
### Systematic Refactoring
136+
137+
#### Line-Based Operations
138+
- **Work bottom-up for coordinate-based edits** - Start with highest line numbers to avoid coordinate shifts
139+
- **Example**: When inlining multiple variables, process line 167 before line 94 to maintain stable targets

‎src/core/services/variable-name-validator.ts‎

Lines changed: 53 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,11 @@
1-
import { Node, ParameterDeclaration, FunctionDeclaration, MethodDeclaration, ArrowFunction, FunctionExpression, BindingElement, OmittedExpression, ConstructorDeclaration, ObjectBindingPattern } from 'ts-morph';
1+
import { Node, ParameterDeclaration, FunctionDeclaration, MethodDeclaration, ArrowFunction, FunctionExpression, BindingElement, OmittedExpression, ConstructorDeclaration } from 'ts-morph';
2+
import { ScopeAnalyzer } from './scope-analyzer';
23

34
type FunctionLikeNode = FunctionDeclaration | MethodDeclaration | ArrowFunction | FunctionExpression | ConstructorDeclaration;
45

56
export class VariableNameValidator {
67
generateUniqueName(baseName: string, scope: Node): string {
7-
const existingNames = this.getExistingVariableNames(scope);
8-
9-
if (!existingNames.has(baseName)) {
8+
if (!this.getExistingVariableNames(scope).has(baseName)) {
109
return baseName;
1110
}
1211

@@ -18,6 +17,7 @@ export class VariableNameValidator {
1817

1918
this.addFunctionParametersIfInBlock(scope, names);
2019
this.addDescendantVariableNames(scope, names);
20+
this.addParentScopeVariableNames(scope, names);
2121

2222
return names;
2323
}
@@ -44,21 +44,48 @@ export class VariableNameValidator {
4444
});
4545
}
4646

47+
private addParentScopeVariableNames(scope: Node, names: Set<string>): void {
48+
let parentScope = ScopeAnalyzer.getParentScope(scope);
49+
while (parentScope) {
50+
this.addDirectVariableNames(parentScope, names);
51+
this.addFunctionParametersIfInBlock(parentScope, names);
52+
parentScope = ScopeAnalyzer.getParentScope(parentScope);
53+
}
54+
}
55+
56+
private addDirectVariableNames(scope: Node, names: Set<string>): void {
57+
scope.forEachChild((child) => {
58+
this.addChildVariableNames(child, names);
59+
});
60+
}
61+
62+
private addChildVariableNames(child: Node, names: Set<string>): void {
63+
if (Node.isVariableStatement(child)) {
64+
child.getDeclarations().forEach((declaration) => {
65+
this.addVariableNameIfExists(declaration, names);
66+
});
67+
} else if (Node.isVariableDeclaration(child)) {
68+
this.addVariableNameIfExists(child, names);
69+
} else if (Node.isParameterDeclaration(child)) {
70+
this.addParameterNameIfExists(child, names);
71+
}
72+
}
73+
74+
private addNameIfIdentifier(node: Node, names: Set<string>): void {
75+
if (Node.isIdentifier(node)) {
76+
names.add(node.getText());
77+
}
78+
}
79+
4780
private addVariableNameIfExists(node: Node, names: Set<string>): void {
4881
if (Node.isVariableDeclaration(node)) {
49-
const nameNode = node.getNameNode();
50-
if (Node.isIdentifier(nameNode)) {
51-
names.add(nameNode.getText());
52-
}
82+
this.addNameIfIdentifier(node.getNameNode(), names);
5383
}
5484
}
5585

5686
private addParameterNameIfExists(node: Node, names: Set<string>): void {
5787
if (Node.isParameterDeclaration(node)) {
58-
const nameNode = node.getNameNode();
59-
if (Node.isIdentifier(nameNode)) {
60-
names.add(nameNode.getText());
61-
}
88+
this.addNameIfIdentifier(node.getNameNode(), names);
6289
}
6390
}
6491

@@ -69,16 +96,13 @@ export class VariableNameValidator {
6996
}
7097

7198
private processParameters(functionNode: Node, names: Set<string>): void {
72-
const parameters = this.getParametersFromFunction(functionNode);
73-
for (const param of parameters) {
99+
for (const param of this.getParametersFromFunction(functionNode)) {
74100
this.processParameter(param, names);
75101
}
76102
}
77103

78104
private getParametersFromFunction(functionNode: Node): ParameterDeclaration[] {
79-
if (Node.isFunctionDeclaration(functionNode) || Node.isMethodDeclaration(functionNode) ||
80-
Node.isArrowFunction(functionNode) || Node.isFunctionExpression(functionNode) ||
81-
Node.isConstructorDeclaration(functionNode)) {
105+
if (this.isFunctionNode(functionNode)) {
82106
return (functionNode as FunctionLikeNode).getParameters();
83107
}
84108
return [];
@@ -87,54 +111,27 @@ export class VariableNameValidator {
87111
private processParameter(param: ParameterDeclaration, names: Set<string>): void {
88112
const nameNode = param.getNameNode();
89113
if (Node.isIdentifier(nameNode)) {
90-
names.add(nameNode.getText());
114+
this.addNameIfIdentifier(nameNode, names);
91115
} else {
92-
this.addDestructuredParameterNames(nameNode, names);
93-
}
94-
}
95-
96-
private addDestructuredParameterNames(nameNode: Node, names: Set<string>): void {
97-
if (Node.isObjectBindingPattern(nameNode)) {
98-
this.addObjectBindingNames(nameNode, names);
99-
} else if (Node.isArrayBindingPattern(nameNode)) {
100-
this.addArrayBindingNames(nameNode, names);
116+
this.addBindingPatternNames(nameNode, names);
101117
}
102118
}
103119

104-
private addObjectBindingNames(nameNode: Node, names: Set<string>): void {
105-
if (Node.isObjectBindingPattern(nameNode)) {
106-
this.processBindingElements(nameNode, names);
107-
}
108-
}
109-
110-
private processBindingElements(nameNode: ObjectBindingPattern, names: Set<string>): void {
111-
nameNode.getElements().forEach((element: BindingElement | OmittedExpression) => {
112-
this.processBindingElement(element, names);
113-
});
114-
}
115-
116-
private processBindingElement(element: BindingElement | OmittedExpression, names: Set<string>): void {
117-
if (!Node.isBindingElement(element)) return;
118-
const elementName = element.getNameNode();
119-
if (Node.isIdentifier(elementName)) {
120-
names.add(elementName.getText());
121-
}
122-
}
123-
124-
private addArrayBindingNames(nameNode: Node, names: Set<string>): void {
125-
if (Node.isArrayBindingPattern(nameNode)) {
126-
nameNode.getElements().forEach((element: BindingElement | OmittedExpression) => {
127-
this.processArrayBindingElement(element, names);
120+
private addBindingPatternNames(pattern: Node, names: Set<string>): void {
121+
if (Node.isObjectBindingPattern(pattern)) {
122+
pattern.getElements().forEach(element => {
123+
this.addBindingElementName(element, names);
124+
});
125+
} else if (Node.isArrayBindingPattern(pattern)) {
126+
pattern.getElements().forEach(element => {
127+
this.addBindingElementName(element, names);
128128
});
129129
}
130130
}
131131

132-
private processArrayBindingElement(element: BindingElement | OmittedExpression, names: Set<string>): void {
132+
private addBindingElementName(element: BindingElement | OmittedExpression, names: Set<string>): void {
133133
if (element && Node.isBindingElement(element)) {
134-
const elementName = element.getNameNode();
135-
if (Node.isIdentifier(elementName)) {
136-
names.add(elementName.getText());
137-
}
134+
this.addNameIfIdentifier(element.getNameNode(), names);
138135
}
139136
}
140137
}

‎tests/fixtures/commands/rename/child-scope-conflict.expected.err‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
Variable name 'x' already exists in this scope. Please choose a different name.

‎tests/fixtures/commands/rename/child-scope-conflict.expected.ts‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
/**
2+
* @description Test rename with child scope conflict - renaming y to x should fail
3+
* @command refakts rename "[child-scope-conflict.input.ts 9:15-9:16]" --to "x"
4+
* @expect-error true
5+
*/
6+
function f() {
7+
const x = 1;
8+
{
9+
const y = 2;
10+
{
11+
return x;
12+
}
13+
}
14+
}

‎tests/fixtures/commands/rename/child-scope-conflict.input.ts‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
/**
2+
* @description Test rename with child scope conflict - renaming y to x should fail
3+
* @command refakts rename "[child-scope-conflict.input.ts 9:15-9:16]" --to "x"
4+
* @expect-error true
5+
*/
6+
function f() {
7+
const x = 1;
8+
{
9+
const y = 2;
10+
{
11+
return x;
12+
}
13+
}
14+
}

0 commit comments

Comments
 (0)