Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sort-comp Allow methods to belong to any matching group. #1858

Merged
merged 4 commits into from
Jul 16, 2018
Merged

sort-comp Allow methods to belong to any matching group. #1858

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2f56e47
sort-comp Allow methods to belong to any matching group.
nosilleg Jun 27, 2018
0b0e462
sort-comp #1858 PR amendments
nosilleg Jun 29, 2018
98ce807
sort-comp #1858 PR amendments
nosilleg Jul 2, 2018
76a4120
sort-comp #1858 PR amendments
nosilleg Jul 4, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
162 changes: 95 additions & 67 deletions lib/rules/sort-comp.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,84 +131,108 @@ module.exports = {
* @returns {Array} The matching patterns indexes. Return [Infinity] if there is no match.
*/
function getRefPropIndexes(method) {
let isRegExp;
let matching;
let i;
let j;
const indexes = [];
const methodGroupIndexes = [];

if (method.getter) {
const getterIndex = methodsOrder.indexOf('getters');
if (getterIndex >= 0) {
indexes.push(getterIndex);
}
}

if (method.setter) {
const setterIndex = methodsOrder.indexOf('setters');
if (setterIndex >= 0) {
indexes.push(setterIndex);
}
}
for (let groupIndex = 0; groupIndex < methodsOrder.length; groupIndex++) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could this be a .map instead of a for loop?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will update this to be a forEach (since we don't care about the resulting array that a map creates).

I had considered making it a forEach, but reading the code I had assumed that fors were preferable since there's a lot of places that forEach could be used but is not.

const currentGroup = methodsOrder[groupIndex];

if (method.typeAnnotation) {
const annotationIndex = methodsOrder.indexOf('type-annotations');
if (annotationIndex >= 0) {
indexes.push(annotationIndex);
}
}

if (indexes.length === 0) {
for (i = 0, j = methodsOrder.length; i < j; i++) {
isRegExp = methodsOrder[i].match(regExpRegExp);
if (isRegExp) {
matching = new RegExp(isRegExp[1], isRegExp[2]).test(method.name);
} else {
matching = methodsOrder[i] === method.name;
switch (currentGroup) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this have to be a switch? it's so much more verbose than if/elses would be.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't have to be a switch.

Generally switch is considered to be more readable and more performant than a chain of if/elsees. For me, the potential conciseness of the if/else chain wouldn't help with readability or maintainability; which would be my main concern.

I don't know the performance characteristics for this in JavaScript, but this article would indicate that it's true for JavaScript as well.

The fallthrough block of lifecycle methods is the most verbose part of this. That could be changed to be a part of the default block, which would allow for the shared array of lifecycle method names (mentioned in another comment) with an .includes(currentGroup). I would say that this is less "pure" than the code as it stands, but not enough to block the suggestion.

Of course, you may just want it to be all if/else.

Let me know which option you want to go with.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I definitely disagree; I consider switch to almost always be less readable than an alternative, and certainly less maintainable. It'd be great to go with all if/else.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will update to be if/else chain.

I would be remiss if I didn't point out that your opinion goes against the popular opinion. e.g. why switch case and not if else if So while this change may be more readable and maintainable for you, it potentially wont be for the masses.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Jordan. It's a shame we have to belabour this point.

I didn't provide JavaScript specific discussions because you didn't specifically mention JavaScript in your opinion. Taking that into consideration is it fair to read your original statement as:

I definitely disagree; I consider switch to almost always be less readable than an alternative, and certainly less maintainable for JavaScript. I agree that it is more readable, maintainable, and performant for other languages. This is the overwhelming shared opinion within the JavaScript community, despite the similarities in syntax between JavaScript and other languages, and the similar performance implications with other languages.

If so, can you please provide references as I'd like to be following community best practices.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we're talking about a JS project, the context "in JavaScript" should be assumed. I offer no opinion here on use of switch in languages irrelevant to this repository (namely, everything that's not JavaScript).

Google, as well as common style guides (jslint, for example, since the 90s; airbnb's; etc), can all be consulted for information on that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mention styleguides, but only link to one person opinion pieces.

The styleguides themselves make no claims that switch should be avoided and instead encourage sane syntax as per every other language that has switch statements.

The opinion pieces that you link to make the main complaints that switch doesn't follow their preferred programming paradigm. I.e. it isn't OO enough (very appropriate for a 2010 opinion piece.), or not functional enough (the new hotness). JavaScript is not a single paradigm language.

You say that JSLint has discourage the use of switch since the 90s, but considering it was created in 2002, and Crockford himself doesn't advocate avoiding switch I'm not sure what to make of your claims.

Others stating that premature deoptimisation is preferable to using using the actual features of a language so that they can pretend it's a different language doesn't seem rational to me. Using JavaScript as JavaScript with the lint enforced best practices does, and the styleguides appear to back this view.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah lol, you're right about jslint, i misremembered.

Either way, avoiding switch statements is still a best practice (in JavaScript, obviously).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please provide sources that confirm it is best practice to avoid switch statements in JavaScript, or help me understand what is incorrect about my last message if you believe your previous sources were sufficient.

case 'getters': {
if (method.getter) {
methodGroupIndexes.push(groupIndex);
}
break;
}
if (matching) {
indexes.push(i);
case 'setters': {
if (method.setter) {
methodGroupIndexes.push(groupIndex);
}
break;
}
}
}

if (indexes.length === 0 && method.static) {
const staticIndex = methodsOrder.indexOf('static-methods');
if (staticIndex >= 0) {
indexes.push(staticIndex);
}
}

if (indexes.length === 0 && method.instanceVariable) {
const annotationIndex = methodsOrder.indexOf('instance-variables');
if (annotationIndex >= 0) {
indexes.push(annotationIndex);
}
}

if (indexes.length === 0 && method.instanceMethod) {
const annotationIndex = methodsOrder.indexOf('instance-methods');
if (annotationIndex >= 0) {
indexes.push(annotationIndex);
}
}

// No matching pattern, return 'everything-else' index
if (indexes.length === 0) {
for (i = 0, j = methodsOrder.length; i < j; i++) {
if (methodsOrder[i] === 'everything-else') {
indexes.push(i);
case 'type-annotations': {
if (method.typeAnnotation) {
methodGroupIndexes.push(groupIndex);
}
break;
}
case 'static-methods': {
if (method.static) {
methodGroupIndexes.push(groupIndex);
}
break;
}
case 'instance-variables': {
if (method.instanceVariable) {
methodGroupIndexes.push(groupIndex);
}
break;
}
case 'instance-methods': {
if (method.instanceMethod) {
methodGroupIndexes.push(groupIndex);
}
break;
}
case 'displayName':
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this seems like a list of lifecycle methods that should be a shared constant array, available in multiple rules

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a sane suggestion, but...

It only works if you want to move to one of the two options above where we remove the lifecycle switch block (or switch completely).

And there is code that can make use of the shared array. (After a quick and rudimentary search I'm not seeing any other candidate code, but it may exist.)

case 'propTypes':
case 'contextTypes':
case 'childContextTypes':
case 'mixins':
case 'statics':
case 'defaultProps':
case 'constructor':
case 'getDefaultProps':
case 'state':
case 'getInitialState':
case 'getChildContext':
case 'getDerivedStateFromProps':
case 'componentWillMount':
case 'UNSAFE_componentWillMount':
case 'componentDidMount':
case 'componentWillReceiveProps':
case 'UNSAFE_componentWillReceiveProps':
case 'shouldComponentUpdate':
case 'componentWillUpdate':
case 'UNSAFE_componentWillUpdate':
case 'getSnapshotBeforeUpdate':
case 'componentDidUpdate':
case 'componentDidCatch':
case 'componentWillUnmount':
case 'render': {
if (currentGroup === method.name) {
methodGroupIndexes.push(groupIndex);
}
break;
}
default: {
// Is the group a regex?
const isRegExp = currentGroup.match(regExpRegExp);
if (isRegExp) {
const isMatching = new RegExp(isRegExp[1], isRegExp[2]).test(method.name);
if (isMatching) {
methodGroupIndexes.push(groupIndex);
}
} else if (currentGroup === method.name) {
methodGroupIndexes.push(groupIndex);
}
break;
}
}
}

// No matching pattern and no 'everything-else' group
if (indexes.length === 0) {
indexes.push(Infinity);
// No matching pattern, return 'everything-else' index
if (methodGroupIndexes.length === 0) {
const everythingElseIndex = methodsOrder.indexOf('everything-else');

if (everythingElseIndex !== -1) {
methodGroupIndexes.push(everythingElseIndex);
} else {
// No matching pattern and no 'everything-else' group
methodGroupIndexes.push(Infinity);
}
}

return indexes;
return methodGroupIndexes;
}

/**
Expand Down Expand Up @@ -409,6 +433,10 @@ module.exports = {

// Loop around the properties a second time (for comparison)
for (k = 0, l = propertiesInfos.length; k < l; k++) {
if (i === k) {
continue;
}

propB = propertiesInfos[k];

// Compare the properties order
Expand Down
Loading