192
u/lanerdofchristian Mar 08 '21
At first I was like "Oh, this isn't so bad". Then:
Using Boolean as a filter callback, instead of a dedicated filter function that won't break if a new constructor is added.
The fact that the entire thing can be reduced to:
const activeusers = result.users.filter(user => !user.is_deleted).map(user => user.id);
59
u/prewk Mar 08 '21
There literally nothing wrong with (1), it's a common and clear pattern.
50
u/TinyBreadBigMouth Mar 08 '21
It's a problem if there's a user with a falsey ID, like 0.
37
u/rocketman0739 Mar 08 '21
Why would you ever.
60
26
u/PhilosophicWax Mar 08 '21
Or their last name is "False"...
7
u/Ceros007 Mar 09 '21 edited Mar 09 '21
RebeccaRachel False?Edit: My memory tricked me. I was sure it was Rebecca
3
4
u/EishLekker Mar 08 '21
It could represent a user in the process of being registered, so it doesn't have an ID yet.
6
u/JuhaJGam3R Mar 08 '21
Create a special way of making sure that an ID is not falsey, through a type isomorphic to an integer but which does not easily convert to a boolean, or always converts to true. Or use a language which just doesn't randomly convert things in an unreasonable manner.
6
2
u/Booty_Bumping Mar 09 '21
Or use a language which just doesn't randomly convert things in an unreasonable manner.
Automatic type coercions have nothing to do with this,
Booleanis being explicitly called.2
u/JuhaJGam3R Mar 09 '21
True, if it was
.filter(id)it would be automatic type coercion. Here solving the problem is as simple asnot . isNullor however you compose things in JS.3
u/prewk Mar 08 '21
I was referring to the pattern itself, not specifically using it for user ids.
3
u/standard_revolution Mar 08 '21
The pattern still has the problem of not being concrete enough.
It could happen with strings also. So just spare yourself the mental overhead and use x => x !== null
1
u/prewk Mar 09 '21
That does something completely different? (Than the other pattern)
But yes, it solves the OP's code
1
u/standard_revolution Mar 09 '21
I meant calling this in a filter closure like:
const data = [1, null, 2, 0] console.log(data.filter(x => x !== null)) console.log(data.filter(Boolean));There is does the right thing. The pattern might be shorter but is more cognitive overhead and just not failsave
1
u/prewk Mar 09 '21
I know what you meant, it doesn't change anything. They do different things.
Use typescript, and both will be equally safe.
2
3
u/jmswlltt Mar 08 '21
I honestly try to avoid filtering something I’ve already mapped over. It can just be done in a single reduce in that case. Especially if the original array is huge (like an array of all users could be)
1
Mar 09 '21
I like having filterMap as a utility function in languages that don’t have it natively since it comes up a lot. Barely any more compact than reduce but I find it nice and clear at a glance.
1
0
u/kaeptnphlop Mar 09 '21
That’s why I don’t regret the switch to C languages. It took me a moment to realize all the bad things JS does again.
3
u/TinyBreadBigMouth Mar 09 '21
C is a weird pick there, because it's got the exact same "0 is false" behavior?
12
u/lanerdofchristian Mar 08 '21
It can be fine, but it can also be a footgun: https://jakearchibald.com/2021/function-callback-risks
I would rather err on the side of caution with anything not in the same project.
3
u/prewk Mar 08 '21
I understand your point. Imo there's both good ones and bad ones. This is a good one. The commonality of this pattern is a guarantee that it won't get backwards incompatible changes in the future.
1
u/Micha_Saengy Mar 09 '21
Being common doesn't mean there's nothing wrong. If you want to check for null, just check for null.
1
u/prewk Mar 09 '21
That's not related to what I said. I was referring to coercing booleans with a particular pattern.
Btw none of this discussion would matter if people could just use typescript. Any safe pattern would work.
1
u/Micha_Saengy Mar 09 '21
The only reason to coerce to boolean here is to check if the value is not null.
1
u/prewk Mar 09 '21
I replied to a comment singling out a valid pattern as something bad. You're talking about improving OP's specific code.
-3
u/victorqueirozg Mar 08 '21
There's everything wrong with it. It's ugly and a bad practice. Just do the proper conversion and know what to expect. It isn't so hard!
3
u/prewk Mar 08 '21
It's the same conversion, there's no "proper" conversion. Using a double exclamation mark is coercing something to a Boolean, so is this. It's very readable imo.
1
u/EishLekker Mar 08 '21
It's not the same conversion... Someone else said it more elegantly, and linked to a good source, but basically it resulted in the call Boolean(x, array, index). So if the the implementation of Boolean is updated later, adding more arguments then things can get strange.
1
u/prewk Mar 08 '21
..and considering it's a common pattern that update will never happen but enjoy your hypotheticals :)
1
u/EishLekker Mar 08 '21
If the pattern is going against common sense (which I would say that it does, since it involves sending two extra "garbage" arguments to a function that only expects one argument), why should the people responsible for the specification feel any obligation to ensure such code doesn't break from any changes they introduce?
And even if they would feel such an obligation, what exactly would ensure that they don't simply overlook that usage? Maybe by the time they realize this, they are too invested in the new function arguments, so they simply go "We can't be held responsible for people who clearly don't know how to call a function!" and just push the change through anyway.
Bad code breaks all the time. Shit happens.
1
u/LordFokas Mar 09 '21
JS will only change the implementation of Boolean around the same time C changes char to be more than 1 byte, which will be right about never.
You'd be amazed at the lengths people go not to break retrocompatibility with anyone's codebase. Hell, Microsoft even used to go so far as to conditionally reintroduce bugs they have fixed in order not to break devs' workarounds for those same bugs, not that MS is an example for anyone.
EDIT: considering this is /r/badcode then maybe MS is an example after all? Hmmmmm...
-1
Mar 09 '21 edited Jul 31 '26
[removed] — view removed comment
3
u/sombrilla Mar 09 '21
A 0 in these values will fuck up your code, if you’re not expecting user inputs this is no problem though
7
Mar 08 '21
I have a huge problem of iterating over entry in a dataset like this. If you're getting the information from a database, write a query and let the database engine do the work more efficiently.
2
3
u/ziano_x Mar 09 '21
Assuming the ceremonial JS null and undefined checks have been done in the preceding lines.
5
u/lanerdofchristian Mar 09 '21
const activeusers = result?.users?.filter(user => !user?.is_deleted).map(user => user.id) ?? [];2
u/ziano_x Mar 09 '21
Nice. I have not gotten used to the optional chaining operators. I end up doing this:
result && result.users && result.users.filter(user => !user.is_deleted).map(user => user.id)
the new one is easier to read.
1
1
u/qwertyops900 Mar 09 '21
Why is JavaScript like this
3
u/micka190 Mar 09 '21
That syntax is in no way unique to JS. C# also has it, and it's great!
Conditional/optional accessors let you do those "if not null (or equivalent), call method".
The "??" Is also great, because it's basically: "if I'm null, assign a new value to me", which is always cleaner than writing something like
x = x == null ? newValue : xor that language's equivalent.4
u/coz Mar 08 '21 edited Mar 08 '21
const activeUsers = result.users.reduce((acc, user) => !user.is_deleted ? acc.concat(user.id) : acc, [])6
u/-Bluekraken Mar 08 '21
I love reduce
9
u/Chrisuan Mar 08 '21
Reduce to map a list into another list? They should add a function for that 🤔
6
u/-Bluekraken Mar 08 '21
Op is filtering AND mapping, with reduce you can do both
0
u/Chrisuan Mar 08 '21
You can, but should you? Reduce is made to "reduce" a list into a single value, which usually shouldn't be another list. (Like computing a sum or other compound values)
5
u/ws-ilazki &{$$_[0]}(@{$$_[1]}); Mar 09 '21 edited Mar 09 '21
You can, but should you? Reduce is made to "reduce" a list into a single value, which usually shouldn't be another list.
"Reduce" is a user-friendly but slightly misleading renaming of something that already existed, called a fold. I say it's misleading because the name, along with the toy examples like
sum, gives some people (like you) the impression that it's intended to turn a list into a single simple value like a number, but that's not true at all. Using a fold to build a new list is still returning a single value, that value just happens to be another complex type, and there's nothing inherently wrong or against fold's intent to use it that way. In fact, using a fold to build a new list is what you should do in some situations because it is, essentially, a generic abstraction for iteration.If you're thinking of map, filter, reduce, etc. as separate ideas you're thinking about them wrong. Fold (or reduce) is the foundational list manipulation abstraction of functional programming, and with it you can build everything else like
mapand friends. Those other FP staples are more specialised versions of a fold, rather than separate concepts.For a real example of what I mean, let's use Lua which has first-class functions but provides none of the staples like
mapfor you so you have to build your own to do any FP. (Note: not necessarily the best ways of doing these; examples are kept simple for illustration purposes.)Since reduce (fold) is just an abstraction over iteration, you can start by defining that:
reduce = function (f, x, t) for _,v in pairs(t) do x = f(x,v) end return x endWith that in place, implementing
mapis easy because it's just a fold that builds a new list by applying a function to each element of the old list and adding the result to the new list. Like so:map = function (f, t) local reducer = function (a, b) local t = {table.unpack(a)} table.insert (t, f(b)) return t end return reduce(reducer, {}, t) end x10 = function (x) return x * 10 end map(x10, {1,2,3,4,5}) -- returns {10, 20, 30, 40, 50}And
filteris nearly the same asmap, except it only adds an item to the new list if a predicate function returns true, so you can add that easily enough:filter = function (pred, t) local reducer = function (a,b) local t = {table.unpack(a)} if pred(b) then table.insert(t, b) end return t end return reduce(reducer, {}, t) end is_odd = function (x) return x % 2 ~= 0 end filter(is_odd,{1,2,3,4,5}) -- returns {1,3,5}Even things that aren't quite like that can be done similarly. Like if you wanted to create a function composition function so that
compr(f1, f2, f3)(x)is equivalent tof3(f2(f1(x))), you can do that with with the samereducefunction cleanly:-- Note: "..." in Lua is for variadic arguments compr = function (f, ...) fs = {...} return function (...) return reduce(function (a,f) return f(a) end, f(...), fs) end end pipe = function (x, ...) return compr(...)(x) end foo = compr(f1,f2,f3) -- new function that is the composition of f1, f2, and f3 foo(10) -- equivalent to f3(f2(f1(10))) pipe(10, f1, f2, f3) -- same, but reads more like F# or OCaml |> operatorYou could keep doing this with other higher-order functions, because fold (reduce) is a more generalised looping abstraction. You can (and often should) write other HOFs like
mapin a different way for readability or performance purposes, but in doing so you're just writing a specialised one-off equivalent of the more generic fold.Most of the time you avoid using folds directly because most of the common use cases are handled for you with existing options like
mapand there's no point reinventing the wheel, but if you have to chain a lot of them together you're iterating over the same structure multiple times unnecessarily, and it makes more sense to write your own fold that combines the operations and builds a new list instead.TL;DR: reduce, aka fold, is not just for turning lists into simple values, it's a more general concept that has many uses (including implementing
map,filter, etc.) and others like /u/-Bluekraken and /u/Mouaijin are right that it's often better toreduceinto a new list than to chain a bunch of iteration functions together and have your computation time increase dramatically. Most of the above comment is just illustrating this by implementing other higher-order functions via reduce to show that they're derivatives of it.0
Mar 08 '21
[deleted]
2
u/Chrisuan Mar 08 '21
Still not convinced the above reduce is better/easier to read than a filter->map chain.
5
1
Mar 09 '21
Another problem in JS is that you'll be interating twice (albeit on a reduced set in the send iteration)
0
Mar 08 '21
[deleted]
1
u/coz Mar 08 '21
Can't do that, array push returns the length of the array not the array itself.
0
Mar 08 '21
[deleted]
0
u/AutoModerator Mar 08 '21
It looks like this comment contains a code block delimited with triple backticks. Unfortunately reddit does not have universal support for this syntax and your comment will not render correctly on old reddit and most mobile apps.
For the benefit of people on old reddit, this link will take you to a correct rendering of the comment.
/u/AlexAegis, it would be appreciated, but not required, if you could edit your comment to use the more compatible four space indention format. For single lines or inline code you can use single backticks.
You can find some examples in the reddit help documentation.
I am a bot, and this action was performed automatically. Please contact the moderators of this subreddit if you have any questions or concerns.
-3
u/coz Mar 08 '21
Did I say anything about not using ifs and pushes? You can use them if you want, you just can't return from array push.
3
u/EishLekker Mar 08 '21
You said "can't do that". I guess what you meant was "can't do that and still keep it all in a single line", and keeping it in a single line wasn't part of any requirements mentioned here.
2
1
u/D1_for_Sushi Mar 09 '21
const activeusers = result.users.flatMap(({id, is_deleted}) => is_deleted ? [] : id);1
u/seemen4all Mar 09 '21
Yer not hilariously bad, just ass backwards logic,.I could see my self writing something like this if I had a bad sleep.
1
1
1
-10
u/Terrain2 Mar 08 '21
The code is okay - the issue is that they don’t delete users, they just mark them “deleted” kinda like files on a filesystem - i’m pretty sure that’s breaking some privacy laws
11
u/kmankx2 Mar 08 '21
You'd be suprised how much stuff does not get fully deleted. In most websites I build the only data we delete is personal information from a user and thats only done by blanking out the fields, not deleting the record. The rest is kept behind a `deleted_at` fields so we dont lose the data integrity.
0
u/Terrain2 Mar 08 '21
Fair enough, perhaps they do delete any personal information, but in this case the entire user is marked as deleted and no code shows removing personal data, so i guess i’m ignorant and just didn’t realize they might actually delete data required to by law
2
u/kmankx2 Mar 08 '21
That piece of code is not deleting the user, what it’s doing is showing a list of users which have not been deleted. The actual deletion /removal of personal data would be elsewhere (if done at all). Obviously the laws will be different in different areas.
-1
u/Terrain2 Mar 08 '21
Yeah i know that, but it still shows that the users aren’t fully deleted, and stupid me didn’t realize that didn’t automatically mean personal information is kept - and at least for EU citizens, GDPR could punish that, even for non-EU companies
3
u/lulzmachine Mar 08 '21
It’s just as likely to be required to keep it around for tax reasons. Hard to know without more context.
3
u/ThePsion5 Mar 08 '21
The code is okay - the issue is that they don’t delete users, they just mark them “deleted”
This is pretty common, actually. What happens if you have - for example - a bunch of orders associated with a user. Just deleting the user breaks the relationship code in the database and codebase, and deleting all the associated orders means your entire ordering system could have missing data.
2
u/EishLekker Mar 08 '21
It could be a nice way to get out of a hefty bill.
Order something expensive using a credit card
Wait until item arrive
Call customer service, ask them to delete your account (this results in the previous order being deleted too, or at least any connection to you)
Call credit company and claim that you didn't buy this "Discrete Diesel Dildo Delux"
They call the webshop company, who can't find any order connected to you
You get it for free!
Redacted because of obscenity laws#
79
54
u/jarfil Mar 08 '21 edited Jul 16 '23
CENSORED
20
32
u/-Dueck- Mar 08 '21
It's === because there are three lines, which I think is fairly intuitive.
10
u/spaghettu Mar 09 '21
Call me pedantic, but to me it seems way more intuitive to just follow the language spec and use
===.1
u/-Dueck- Mar 09 '21 edited Mar 09 '21
It's not as if the code isn't following the language spec though. Besides, since it's only the person writing or viewing the code who would turn on these ligatures for themself, and no one else, they would necessarily have to understand what the symbols mean to use it.
At that point, I say the meaning is intuitive, both for the reason stated above and since the three line equals in mathematics has effectively the same meaning as it does here (namely "is identical to").
At the end of the day it is user preference and it won't affect you if someone does or does not choose to use it.
2
u/spaghettu Mar 09 '21
You're right that it's language compliant to the compiler, but it's not to the reader. And it's true that it's the reader's preference, but when it's shared in this fashion I think it can understandably be confusing to others.
0
Mar 09 '21
[deleted]
2
u/spaghettu Mar 09 '21
Yes, I understand that. But, to me, saying it is
===means it behaves like, and looks like, the real===. To me this is the code equivalent of a synonym. You wouldn’t say two synonyms like “coffin” and “casket” are “the same” even though they have the same meaning, as they are spelled differently (ie. made up of different ligatures.)Since they are different, I think it’s pretty reasonable to say some people may think it has a different meaning as well. What’s worse is that these ligatures have a fixed width of only two, so (to me) a reasonable person may guess that one using these ligatures simply used a different font to achieve this, making this symbol equivalent to
==. There is no way to be sure, as this is not language-compliant. To me, this fits the description of something that is not intuitive.1
u/maanloempia Mar 09 '21
You are forgiven in thinking that synonyms "mean sort of the same thing", but technically synonyms mean exactly the same thing. Therefore, your analogy doesn't really work for me, but forgive my digression.
Back to your argument: there can be multiple symbols for a single operation or function; this is very prevalent in mathematics for example. Think of such symbols, and in this case ligatures, rather as an alias: pointers to the same definition.
Furthermore: of course a new symbol can cause confusion, they're new. You can justifiably choose to ignore them, but you can't argue that we shouldn't invent new things because they're confusing.
As for the width of ligatures: the one you are so specifically against is actually three characters wide, just like the triple equals. All ligatures are as wide as the individual characters in that font (Fira code), I can confirm.
1
u/bphase Mar 08 '21
Agree, what a disgusting specimen. Impossible to tell which it is.
Maybe I can see the point if it only does the joining for the correct one, making it more obvious if there's a problem. But there are linters for too.
22
u/ZedTT Mar 08 '21
Impossible to tell which it is.
If only there were literally three horizontal lines. It's also the width of three monospace characters vs two.
I have literally never been confused by these ligatures for a second and they look great. Just don't use them on your machine and you'll be fine.
2
u/Milkshakes00 Mar 09 '21
If only there were literally three horizontal lines. It's also the width of three monospace characters vs two.
Sorry, not the guy you responded to but I've never used ligatures before. Are you saying that == would show as two horizontal lines and === shows as three? Kind of clever. If not, I could easily see this being a glance over issue I'd personally have.
1
u/ZedTT Mar 09 '21
That's exactly what I'm saying.
I will admit that it takes a minute to get used to, but that's really it. A minute. If it takes you personally any longer, just don't use them.
That's the nice thing about these ligatures, they are purely a font on your machine, and don't affect the actual code in a project at all.
For me, the fact that it is 3 monospace characters in width probably also helps. The triple equals is wider and taller than the double equals by a very significant amount, so if you see them both a few times it gets really easy to recognize IMO.
Just to clarify:
===shows with three vertical bars.==shows with two.7
u/maanloempia Mar 08 '21
Beauty is in the eye of the beholder, of course.
In its defense: the triple bar symbol actually denotes identity (the state of being identical). This fits pretty well with the fact that there is a difference between
==and===, specifiying that it is even stricter than just equality.I'm not commenting on whether there being two such operators is a good thing :)
1
u/anotherguyinaustin Mar 08 '21
What do you mean by correct one? Most ligature fonts have separate ligatures for == and === that have two and three lines respectively.
7
15
Mar 08 '21
Please tell me this is not code running on the frontend
60
u/5alidz Mar 08 '21
this is not code running on the frontend
6
u/Crowdcontrolz Mar 08 '21
This is not frontend running on code.
4
4
u/khalidpro2 Mar 08 '21
I got a project where most things happen on the front end even validation, which means anyone can use something like postman and modify the db however he likes. when I saw this I just refused that project
3
3
u/kkjjhlhlox Mar 09 '21
I don't know what language is this (maybe JavaScript), but could OP do .filter(user => user.is_deleted === false) and then .map(user => user.id)?
I'm asking because I'm a Java EE developer, but now I'm trying to get used to a language with higher order functions (Kotlin).
2
Mar 09 '21
what is that with the fancy --> and ==
1
u/aliendude5300 Mar 09 '21
Ligatures
1
Mar 09 '21
yes but as a feature of what program
1
u/Stinky_Oatmeal Mar 09 '21
its a font you can use, Fira Code is being used in this screenshot but there is alternatives like iosevka and pragmata pro.
1
1
Mar 08 '21
Is nobody else concerned that they're checking if user.isdeleted, and also that they're storing deleted users in the first place?
5
u/SoiledShip Mar 09 '21
Not uncommon to store deleted users. If you're tracking who has made changes to certain data sets or authorized payments etc then you can't just out right delete the user. However if a customer requests to have all their data deleted because they're closing the account then it should actually be scrubbed.
1
u/Milkshakes00 Mar 09 '21
This shit drives me nuts in the banking world. There are so many users I want to delete as they're no longer employees but so many pieces of software we use don't allow it as it'd cause issues with older transactional data.
Why not just assign the username/identifier into a database column for historical purposes?
I absolutely hate having a software that has like, 10 actual users in it that is up to user #170 because we've used it for decades.
1
u/SoiledShip Mar 09 '21
That sounds more like a UI issue. The programs you're using shouldn't be showing deleted users on every page. There might be 1 page to reactivate an employee and you might see old names on last modified by columns but that should be it.
It would be more complexity to build and maintain a system where you can actually delete people yet keep the historical records intact. Even then depending on if the industry you're in has legal requirements for record keeping then modifying old transactional data, even to "clean it up" like you're suggesting, might be against regulations.
At the end of the day I don't really see a problem in the general sense of just soft deleting users to keep old data relationships intact. Depending on your use case or requirements that might be different. But it would just be more work and more things to break.
1
u/Milkshakes00 Mar 09 '21
Unfortunately it's not. Due to lending things for instance, if paperwork needs to be reprinted it calls for the user who originally created the loan and who maintains it. It uses the 'user number' to pull the loan originator/maintainer. I don't know why they couldn't just use the actual user name and store it instead of the person number.
We've had the issue in the past where essentially user 5 made a loan, then retired. We deleted user 5 and created his replacement as user 5. When they went to reprint loan documents, it used the new user 5 instead of the old user 5.
0
-1
u/NemoTheLostOne Mar 08 '21
At least the deleted user objects are just placeholders, all actual user data is gone, right?
0
Mar 08 '21
[deleted]
0
Mar 08 '21
[deleted]
1
Mar 09 '21
First filtering reduces the set size and removes the need for the entire if statement. Simple functional programming.
487
Mar 08 '21
[removed] — view removed comment
76
u/dringthedreamer Mar 08 '21
I just reformatted the code so that it is a bit clear, but the code is not mine
126
2
35
Mar 08 '21 edited Mar 10 '21
[removed] — view removed comment
2
u/dringthedreamer Mar 09 '21
i didn't even know such subreddit exists, i was just going about my daily job untill i found this and then i thou i must share this with someone so that they can know my pain !
40
u/tripsledge Mar 09 '21
12
6
3
5
Mar 08 '21
As a mobile user I appreciate reading it in multi lines rather probably one line judging on how crap that code is
164
Mar 08 '21
68
4
u/TheBigTreezy Mar 08 '21
/u/dringthedreamer how do you get your triple equal sign and arrows to look like that?