56
u/dethstrobe May 06 '21
A quick performance check. https://codesandbox.io/s/awesome-jones-2fpy1?file=/src/index.js
long hand: 0.12499996228143573
short hand 1: 0.7600000244565308
short hand 2: 0.1700000138953328Long hand is faster because you don't need to waste time and memory space building an array. Premature optimization is the root of all evil. Just keep things simple.
21
u/inabahare May 06 '21
A few things to keep in mind
0.12 and 0.17 are so close that you only have things shorthand 2. It will semantically make more sense.
With the above in mind, you're doing your comparison 2k times. Like you're sacrificing readability and scalability for 0.05? Like that's what they mean by premature optimizaiton being the root of all evil
Those are milliseconds. Like 2k runs in 0.17 milliseconds.. Like that's not a lot of time at all
4
u/dethstrobe May 06 '21
Totally agree. Don’t split hairs on how to do this. If someone really wants to do wacky array includes it chain or operators it’s probably not worth your time to refactor.
2
u/inabahare May 06 '21
* semantically clearer and more scalable code
2
u/dethstrobe May 06 '21
For 4 variable checks? I won’t bother. It’s super debatable if it’s more readable or scalable.
3
u/inabahare May 07 '21
It literally is. Since its an array you can bind it to a variable and boom, readability as fuck. So when the customer says they want more values for whatever it's doing then you don't have to find an if somewhere.
It literally is more scalable. You can dynamically add and remove values and you can store them in a database for even more dynamic fun times
2
u/jbonesinthecloset May 07 '21
Thanks for this man. I’ve been pretty fuzzy on what scalability means in literal terms and this example makes total sense to me. Dynamics!
1
u/RoguePlanet1 May 07 '21
Veering a bit off-topic: I'm now wondering what the code would be to eliminate the "Likes" if they're the first word of the sentence. I should brush up on my Regex probably.
That aside, you make good points.
2
u/inabahare May 07 '21
Surprisingly not so hard, though you do end up loosing a lot of the meaning of the sentence :v
1
1
May 10 '21 edited May 10 '21
I can make it 0.09 if it does matter for you.
https://codesandbox.io/s/competent-visvesvaraya-qqci5?file=/src/index.js:418-573
just make a dictionary and get your value. what's the point? well in this situation nothing besides being faster. in a particular situation? avoiding checking every single condition if you already know what you are checking for. Every book should get rid of switch condition, and you should avoid that much if statements in your methods. imho
71
u/blafurznarg May 06 '21 edited May 06 '21
rant /
Bruh the learnJS community (not just reddit) is being gaslighted and manipulated by so called mentors and teachers trying to be smart. The early leaners being impressed by every destructuring stunt is not gonna help.
Please never write think about the advantages and disadvantages before writing something like that in prod. If you sacrifice readability for cleverness and shortness at all costs your code will become unmaintainable.
Those who can, do; those who can't, teach [and make instagram posts with useless info].
from I don't fucking know
/ rant end sorry
Edit: Cheers to the deleted guy down there. He's right.
23
u/asiraky May 06 '21
“Please never write something like that in prod”.
Nonsense.
I can think of plenty of times where Ive needed to check a range of conditions in non-performance critical code, and I simply use an includes or a some.
8
u/blafurznarg May 06 '21
You're right. There is of course a usecase for that. I guess I was triggered by the post's presentation plus the reaction in the comments. Maybe I shouldn't have emphazised on that particular part to never use it.
4
u/asiraky May 06 '21
And I agree with your general sentiment that shorter code isn’t necessarily better.
3
u/delventhalz May 06 '21
While there are use cases for
includes, I tend to agree that the way it is presented here is not "prod" ready. If I was reviewing a PR where someone created a new throw-away array just to do anincludescheck, I would probably request it be changed.1
u/asiraky May 06 '21
But what is the actual reason for the rejection? The readability of that code may be higher depending on what was being checked. It’s completely reasonable to say ‘if x is in this set’ is more readable than ‘if x is a or x is b or x is c or x is d’. And if it’s not performance critical code (which most code is not, let’s not start a thing about premature optimisation) then I can’t see a reason to reject it. I personally like it when my devs try to be “clever”. At least it means they are thinking about what they’re doing and trying to write good code.
1
u/delventhalz May 07 '21
I wrote a longer top level comment about my reasoning, but the short version is:
“Save it for code golf. Shorter does not always mean simpler or more clear. In this case, sticking to standard conditional chaining more clearly communicates your intent than includes.”
1
u/jbonesinthecloset May 07 '21
That’s still not really a reason for why specifically this one example is better for doing it your way... the intent is clearly communicated your way AND doing it with includes. Your top level comment definitely gives some valid points on why your preferred method does communicate your intent well. But not why it’s MORE clear than using includes
1
u/delventhalz May 07 '21
It's a small point. Depending on the review and the context, I might let it slide. OP's example is also a little too generic to really understand the use case.
All that said, I interpret the use case to be a complex multi-part conditional rather than a defined set of options. And in that case, I do think chaining conditionals pretty definitely requires less cognitive leaps. I can easily imagine juniors looking at that array and going "wait, why is there an array now?"
And while the cognitive load is smallish to context switch from "I am checking a series of conditionals" to "I am checking membership in a set", I don't really see any benefit to adding the extra complication.
1
u/jbonesinthecloset May 08 '21
But like...it is clear why there is an array...
1
u/delventhalz May 08 '21
To you?
1
u/jbonesinthecloset May 08 '21
Based on the comments for this entire post, it’s clear to pretty much everyone except you and your juniors
→ More replies (0)4
u/toastertop May 06 '21 edited May 07 '21
Still read posts etc. just don't get too hung up on things. Refer back to MDN or even Spec for getting info.
I encourage people to get conformable reading spec. I understand the UI/UX is well, not great, for now just find it on MDN then click the link to spec.
It's worded differently so can be sometimes easier to understand and you can trace a line of thought of how something came to be, back up through the versions of the spec. Element is old and a great start [Edit: ca't sp] Also: CanIUse
1
May 07 '21
I agree. It's nice to know different ways but be very careful what you use and where.
Not so long ago I had to fix some of my code because I used let instead of var to declare some variables inside some block statement and some legacy headless browsers didn't support it.
12
u/delventhalz May 06 '21 edited May 06 '21
What a contentious thread!
For all of the people encountering this use of includes for the first time: this is a neat trick! Keep it in your back pocket. I think it is particularly useful when you have a static set of options you need to check something against. Like this example:
const FRUIT = ['apple', 'banana', 'orange'];
function isFruit(item) {
return FRUIT.includes(item);
}However, the heading seems to imply that this is a good general purpose replacement for checking multiple conditions, and I would caution against that impression.
Beware the allure of shorter code. Shorter code is not necessarily simpler or more readable. In OP's code snippet, two new pieces of syntax (the array and it's method) are introduced. More different syntax means more complexity.
Perhaps more importantly, you are stretching the new syntax a bit outside of it's typical use. The "semantic" goal of the code is to check multiple conditions. The purpose of || is to join multiple conditions. Totally aligned. Totally expected.
What is the purpose of includes? To check if an array includes a value. What array? Our code doesn't have an array. Okay, so we make an array so we can force includes to work for us. Think about the complexity of that for a moment. We are making a throw-away array for purely mechanical reasons. A good goal when programming is to try to avoid code that is purely mechanical. Instead try to make your code more "declarative". In other words, write code that just tells you what it does instead of getting into mechanical details. From that perspective, the || actually beats includes in this case.
For what it's worth, I typically solve this sort of complex multi-conditional by making the code longer. No new syntax, I just add whitespace.
const isOneOrTwo = value === 1
|| value === 'one'
|| value === 2
|| value === 'two;
if (isOneOrTwo) {
// Execute some code
}I like this approach for a few reasons. It puts each condition on its own line. Like a nice bullet-pointed list. It also allows me to give the condition a meaningful name, which hopefully clarifies its purpose. Finally, it quarantines the conditional off from the rest of the code. If someone wants to understand the conditional, they can look at it in isolation. If someone wants to understand the if-block, they don't have to worry about the details of the condition.
TLDR: Shorter is not the same as simpler. Stick to what is expected when possible. This is a neat trick, but chaining || and && is probably better most of the time.
2
u/TheMcGarr May 06 '21
It's actually way more useful when it is a dynamic set of options and in fact the long hand doesn't work in that case
3
u/delventhalz May 06 '21
If it is a dynamic set of options, then don't you already have an array you are working with?
The post is about checking "multiple conditions". If we are actually talking about checking whether or not an array includes an item, then yes,
includesis absolutely the right choice.-3
u/TheMcGarr May 06 '21
Not necessarily. A beginner might do something like have different functions with different sets of possible conditions. It isn't immediately obvious that you can translate logical conditions into an array in this way. For me that is what is useful about this post.
19
May 06 '21
There are two paths a developer can follow to make the code "better".
On the first path, they use all sorts of silly syntactic shortcuts to make their code more compact, sometimes calling it more concise.
On the second path, they may sometimes make the code shorter or other times longer, because their aim is to add semantic expressiveness.
If you want to be a better developer, follow the second path. I mean, sure, do make your code "concise" if it makes sense, but never sacrifice expressiveness for compactness.
Btw, one telling sign of a bad "better code" example is that it expresses no meaning, that it only shows syntactic traits. If usually presents a completely synthetic code which intentionally removes any meaning or intention.
11
u/superluminary May 06 '21
But shorthand 2 is legitimately better here. “Is the value in this set” is better than “is the value this or is the value this or...”
5
u/delventhalz May 06 '21
It is a little hard to judge from this code example. What is the use case for checking if something is 1, one, 2, or two? Is that actually some sort of "set" they are checking membership against? Or is this a hacky way to make a long conditional shorter?
I would agree that if the use case is actually checking the membership of set, then
includesis the best choice. But typically if that is the case, you would express the set as a constant at the top of the file or something:const VALID_INPUTS = [1, 'one', 2, 'two'];The use of the throw-away array is what makes this look to me more like a hack than an actual good use of
includes.3
u/superluminary May 06 '21
I agree, I’d definitely make a const here.
If (validInputs.includes(value)) {
21
May 06 '21
or if you want to be super clever:
switch(value) {
case 1:
case 'one':
case 2:
case 'two':
// if
break;
default:
// else
}1
5
May 06 '21
I might argue if you have this many or conditions you might want to see if you can refactor. Not possible in all cases, but should be considered. Having this many conditions that can branch makes the code harder to keep track of in your head.
8
u/Dan6erbond May 06 '21 edited May 06 '21
!== -1 for indexOf() checks is more semantic IMHO because you're being clear that you only care about it existing, and not really the index even if it has the same result.
3
8
u/throwaway1253328 May 06 '21
Longhand is still better here. Why arbitrarily add the values to arrays and then call the array methods? The first one makes way more sense if a third party reads this at a later point
3
u/burnblue May 06 '21
"list of things" .includes 'thing' is more immediately readable (it literally allows your eyes to do less work) and it makes sense. Even a novice relying on intuition might understand 'includes' before '||'
The indexOf is trash though
3
u/skamunism May 06 '21
I adopted Shorthand 2 when learning JS from a SQL background; I wanted the functionality of "value IN ()".
5
2
u/noXi0uz May 06 '21
I would suggest to use Array.some() instead of includes here, as it's more flexible than just string comparison. Although I would personally prefer the "longhand" anyway.
2
u/IlliterateJedi May 06 '21
It seems to me like the Longhand version is most readable. Shorthand 2 is a close second. The only reason I don't really like it is that you have to read ahead past the array to understand what the code is doing. Then once you see the 'includes' method you have to go back and look at the array to see what the conditionals are.
2
u/cinnamonbreakfast May 06 '21
It's simple: longhand when you don't give a shit, shorthand 2 when you too lazy and you don't want to write a lot
2
2
u/BRVH991 Jul 13 '21
Lol total brainlets in the comments. "T-this isn't what I'm used to waaah what does it mean"
5
5
u/kattenmusentiotusen May 06 '21
Shorter is not always better. This will confuse people.
7
1
u/superluminary May 06 '21
Option 2 will not confuse. It translates to “is the value included in this set”.
6
1
u/pekkalacd May 07 '21
i like the shorthand versions. but that triple equals, gross. lol i know it has to be done, but still. if only == worked as other languages use it.
1
u/mynamesleon May 06 '21
The bottom one is the most concise.
However, you need a polyfill or a transpiler for IE support (if you're unfortunate like me, and still have to support it). And if you're trying to get every bit of performance that you can, I'm pretty sure it's the worst of the three!
1
0
37
u/senocular May 06 '21
One minor difference between shorthand 1 and 2 is that 1 (using indexOf) compares with strict equality whereas 2 (using includes) uses the sameValueZero algorithm. So for example: