Skip to content

Commit b25962e

Browse files
fiskerfisker
authored
prefer-string-slice: Refactor (#1667)
1 parent 22d8d03 commit b25962e

File tree

1 file changed

+92
-112
lines changed

1 file changed

+92
-112
lines changed

‎rules/prefer-string-slice.js‎

Lines changed: 92 additions & 112 deletions
Original file line numberDiff line numberDiff line change
@@ -10,8 +10,11 @@ const messages = {
1010
[MESSAGE_ID_SUBSTRING]: 'Prefer `String#slice()` over `String#substring()`.',
1111
};
1212

13-
const substrCallSelector = methodCallSelector({method: 'substr', includeOptionalMember: true, includeOptionalCall: true});
14-
const substringCallSelector = methodCallSelector({method: 'substring', includeOptionalMember: true, includeOptionalCall: true});
13+
const selector = methodCallSelector({
14+
methods: ['substr', 'substring'],
15+
includeOptionalMember: true,
16+
includeOptionalCall: true,
17+
});
1518

1619
const isLiteralNumber = node => node && node.type === 'Literal' && typeof node.value === 'number';
1720

@@ -34,143 +37,120 @@ const isLengthProperty = node => (
3437
&& node.property.name === 'length'
3538
);
3639

37-
/** @param {import('eslint').Rule.RuleContext} context */
38-
const create = context => {
39-
const sourceCode = context.getSourceCode();
40+
function getFixArguments(node, context) {
41+
const argumentNodes = node.arguments;
4042

41-
return {
42-
[substrCallSelector](node) {
43-
const objectNode = node.callee.object;
44-
const argumentNodes = node.arguments;
43+
if (argumentNodes.length === 0) {
44+
return [];
45+
}
4546

46-
const problem = {
47-
node,
48-
messageId: MESSAGE_ID_SUBSTR,
49-
};
47+
const sourceCode = context.getSourceCode();
48+
const firstArgument = argumentNodes[0] ? sourceCode.getText(argumentNodes[0]) : undefined;
49+
const secondArgument = argumentNodes[1] ? sourceCode.getText(argumentNodes[1]) : undefined;
5050

51-
const firstArgument = argumentNodes[0] ? sourceCode.getText(argumentNodes[0]) : undefined;
52-
const secondArgument = argumentNodes[1] ? sourceCode.getText(argumentNodes[1]) : undefined;
51+
const method = node.callee.property.name;
5352

54-
let sliceArguments;
53+
if (method === 'substr') {
54+
switch (argumentNodes.length) {
55+
case 1: {
56+
return [firstArgument];
57+
}
5558

56-
switch (argumentNodes.length) {
57-
case 0: {
58-
sliceArguments = [];
59-
break;
60-
}
59+
case 2: {
60+
if (firstArgument === '0') {
61+
const sliceCallArguments = [firstArgument];
62+
if (isLiteralNumber(secondArgument) || isLengthProperty(argumentNodes[1])) {
63+
sliceCallArguments.push(secondArgument);
64+
} else if (typeof getNumericValue(argumentNodes[1]) === 'number') {
65+
sliceCallArguments.push(Math.max(0, getNumericValue(argumentNodes[1])));
66+
} else {
67+
sliceCallArguments.push(`Math.max(0, ${secondArgument})`);
68+
}
6169

62-
case 1: {
63-
sliceArguments = [firstArgument];
64-
break;
70+
return sliceCallArguments;
6571
}
6672

67-
case 2: {
68-
if (firstArgument === '0') {
69-
sliceArguments = [firstArgument];
70-
if (isLiteralNumber(secondArgument) || isLengthProperty(argumentNodes[1])) {
71-
sliceArguments.push(secondArgument);
72-
} else if (typeof getNumericValue(argumentNodes[1]) === 'number') {
73-
sliceArguments.push(Math.max(0, getNumericValue(argumentNodes[1])));
74-
} else {
75-
sliceArguments.push(`Math.max(0, ${secondArgument})`);
76-
}
77-
} else if (
78-
isLiteralNumber(argumentNodes[0])
79-
&& isLiteralNumber(argumentNodes[1])
80-
) {
81-
sliceArguments = [
82-
firstArgument,
83-
argumentNodes[0].value + argumentNodes[1].value,
84-
];
85-
} else if (
86-
isNumber(argumentNodes[0], context.getScope())
87-
&& isNumber(argumentNodes[1], context.getScope())
88-
) {
89-
sliceArguments = [firstArgument, firstArgument + ' + ' + secondArgument];
90-
}
91-
92-
break;
73+
if (argumentNodes.every(node => isLiteralNumber(node))) {
74+
return [
75+
firstArgument,
76+
argumentNodes[0].value + argumentNodes[1].value,
77+
];
9378
}
94-
// No default
95-
}
9679

97-
if (sliceArguments) {
98-
const objectText = getParenthesizedText(objectNode, sourceCode);
99-
const optionalMemberSuffix = node.callee.optional ? '?' : '';
100-
const optionalCallSuffix = node.optional ? '?.' : '';
80+
if (argumentNodes.every(node => isNumber(node, context.getScope()))) {
81+
return [firstArgument, firstArgument + ' + ' + secondArgument];
82+
}
10183

102-
problem.fix = fixer => fixer.replaceText(node, `${objectText}${optionalMemberSuffix}.slice${optionalCallSuffix}(${sliceArguments.join(', ')})`);
84+
break;
10385
}
86+
// No default
87+
}
88+
} else if (method === 'substring') {
89+
const firstNumber = argumentNodes[0] ? getNumericValue(argumentNodes[0]) : undefined;
90+
switch (argumentNodes.length) {
91+
case 1: {
92+
if (firstNumber !== undefined) {
93+
return [Math.max(0, firstNumber)];
94+
}
10495

105-
context.report(problem);
106-
},
107-
108-
[substringCallSelector](node) {
109-
const objectNode = node.callee.object;
110-
const argumentNodes = node.arguments;
111-
112-
const problem = {
113-
node,
114-
messageId: MESSAGE_ID_SUBSTRING,
115-
};
96+
if (isLengthProperty(argumentNodes[0])) {
97+
return [firstArgument];
98+
}
11699

117-
const firstArgument = argumentNodes[0] ? sourceCode.getText(argumentNodes[0]) : undefined;
118-
const secondArgument = argumentNodes[1] ? sourceCode.getText(argumentNodes[1]) : undefined;
100+
return [`Math.max(0, ${firstArgument})`];
101+
}
119102

120-
const firstNumber = argumentNodes[0] ? getNumericValue(argumentNodes[0]) : undefined;
103+
case 2: {
104+
const secondNumber = getNumericValue(argumentNodes[1]);
121105

122-
let sliceArguments;
106+
if (firstNumber !== undefined && secondNumber !== undefined) {
107+
return firstNumber > secondNumber
108+
? [Math.max(0, secondNumber), Math.max(0, firstNumber)]
109+
: [Math.max(0, firstNumber), Math.max(0, secondNumber)];
110+
}
123111

124-
switch (argumentNodes.length) {
125-
case 0: {
126-
sliceArguments = [];
127-
break;
112+
if (firstNumber === 0 || secondNumber === 0) {
113+
return [0, `Math.max(0, ${firstNumber === 0 ? secondArgument : firstArgument})`];
128114
}
129115

130-
case 1: {
131-
if (firstNumber !== undefined) {
132-
sliceArguments = [Math.max(0, firstNumber)];
133-
} else if (isLengthProperty(argumentNodes[0])) {
134-
sliceArguments = [firstArgument];
135-
} else {
136-
sliceArguments = [`Math.max(0, ${firstArgument})`];
137-
}
116+
// As values aren't Literal, we can not know whether secondArgument will become smaller than the first or not, causing an issue:
117+
// .substring(0, 2) and .substring(2, 0) returns the same result
118+
// .slice(0, 2) and .slice(2, 0) doesn't return the same result
119+
// There's also an issue with us now knowing whether the value will be negative or not, due to:
120+
// .substring() treats a negative number the same as it treats a zero.
121+
// The latter issue could be solved by wrapping all dynamic numbers in Math.max(0, <value>), but the resulting code would not be nice
138122

139-
break;
140-
}
123+
break;
124+
}
125+
// No default
126+
}
127+
}
128+
}
141129

142-
case 2: {
143-
const secondNumber = getNumericValue(argumentNodes[1]);
130+
/** @param {import('eslint').Rule.RuleContext} context */
131+
const create = context => {
132+
const sourceCode = context.getSourceCode();
144133

145-
if (firstNumber !== undefined && secondNumber !== undefined) {
146-
sliceArguments = firstNumber > secondNumber
147-
? [Math.max(0, secondNumber), Math.max(0, firstNumber)]
148-
: [Math.max(0, firstNumber), Math.max(0, secondNumber)];
149-
} else if (firstNumber === 0 || secondNumber === 0) {
150-
sliceArguments = [0, `Math.max(0, ${firstNumber === 0 ? secondArgument : firstArgument})`];
151-
} else {
152-
// As values aren't Literal, we can not know whether secondArgument will become smaller than the first or not, causing an issue:
153-
// .substring(0, 2) and .substring(2, 0) returns the same result
154-
// .slice(0, 2) and .slice(2, 0) doesn't return the same result
155-
// There's also an issue with us now knowing whether the value will be negative or not, due to:
156-
// .substring() treats a negative number the same as it treats a zero.
157-
// The latter issue could be solved by wrapping all dynamic numbers in Math.max(0, <value>), but the resulting code would not be nice
158-
}
134+
return {
135+
[selector](node) {
136+
const problem = {
137+
node,
138+
messageId: node.callee.property.name,
139+
};
159140

160-
break;
161-
}
162-
// No default
141+
const sliceCallArguments = getFixArguments(node, context);
142+
if (!sliceCallArguments) {
143+
return problem;
163144
}
164145

165-
if (sliceArguments) {
166-
const objectText = getParenthesizedText(objectNode, sourceCode);
167-
const optionalMemberSuffix = node.callee.optional ? '?' : '';
168-
const optionalCallSuffix = node.optional ? '?.' : '';
146+
const objectNode = node.callee.object;
147+
const objectText = getParenthesizedText(objectNode, sourceCode);
148+
const optionalMemberSuffix = node.callee.optional ? '?' : '';
149+
const optionalCallSuffix = node.optional ? '?.' : '';
169150

170-
problem.fix = fixer => fixer.replaceText(node, `${objectText}${optionalMemberSuffix}.slice${optionalCallSuffix}(${sliceArguments.join(', ')})`);
171-
}
151+
problem.fix = fixer => fixer.replaceText(node, `${objectText}${optionalMemberSuffix}.slice${optionalCallSuffix}(${sliceCallArguments.join(', ')})`);
172152

173-
context.report(problem);
153+
return problem;
174154
},
175155
};
176156
};

0 commit comments

Comments
 (0)