I would certainly rather see this than {isAdmin: bool; isLoggedIn: bool}. With boolean | null, at least illegal states are unrepresentable... even if the legal states are represented in an... interesting way.

[–] nialv7@lemmy.world 3 points 1 month ago (1 children)

Admin false LoggedIn false doesn't feel illegal to me, more redundant if anything

[–] shape_warrior_t@programming.dev 9 points 1 month ago* (last edited 1 month ago) (2 children)

I was thinking of the three legal states as:

not logged in (null or {isAdmin: false, isLoggedIn: false})
logged in as non-admin (false or {isAdmin: false, isLoggedIn: true})
logged in as admin (true or {isAdmin: true, isLoggedIn: true})

which leaves {isAdmin: true, isLoggedIn: false} as an invalid, nonsensical state. (How would you know the user's an admin if they're not logged in?) Of course, in a different context, all four states could potentially be distinctly meaningful.

[–] nialv7@lemmy.world 4 points 1 month ago

ah you are right! i am so dumb.

[–] chocrates@piefed.world 1 points 1 month ago

Honestly logged in is state and shouldn't be on the user object.

[–] Drewmeister@lemmy.world 8 points 1 month ago* (last edited 1 month ago) (1 children)

E: omg forget my whole comment. I agree with you that the name sucks.

I mostly don't like that role is typically an intuitive name, and now suddenly it means something I wouldn't expect. Why add confusion to your code? I don't always remember what I meant week to week, much less if someone else wrote it.

[–] dohpaz42@lemmy.world 7 points 1 month ago

If I had a nickel for every time that happened to me, I’d still be poor, but at least I’d have several nickels. 😁

[–] grrgyle@slrpnk.net 7 points 1 month ago

The variable name is 90% why this is so unreasonable. Code is for humans to read, so names matter.

[–] normalexit@lemmy.world 3 points 1 month ago* (last edited 1 month ago) (1 children)

Product manager: "I want a new role for users that can only do x,y,z"

Developer: "uh.. yeah. About that... Give me a few days."

[–] grrgyle@slrpnk.net 5 points 1 month ago

Hmmm I need a datatype with three states... Should I use a named enum? No, no that's too obvious...

[–] orgrinrt@lemmy.world -1 points 1 month ago* (last edited 1 month ago)

Yeah let’s use a union of a boolean and null to represent role, something that inherently represents more than two (…or three, I guess) different values, as opposed to something like an integer.

Even if the name is clearly misleading in this specific case, the entire choice of using a bool here is just bad because it’s almost guaranteed you’re going to expand on that in future and then you’ll just have to entirely rewrite the logic because it simply can’t accommodate more than two values (or three with the null union… 🙈), while it gives absolute zero benefits over using something more reasonable like an integer to represent the roles, or in this case, admin, not-admin and guest. Even if you’ll end up with just admin, non-admin and guest, the integer would still work great with no disadvantages in terms of amount of code or whatever. Just increased legibility and semantical accuracy.

Not to mention that there’s zero reason to combine the state of being logged in and the role in which you’re logged in in one variable… those are two different things. They will remain two different things in future too…

I mean they’re already chaining elseifs (basically matching/switching, while doing it in an inefficient way to boot 🥴) as though there were an n amount of possible states. Why not just make it make sense from the start instead of whatever the hell this is?