This is not a joke. This is not something I wrote to be funny. This is not found randomly off the internet. This is a real part of the project I inherited: a function that not only is more cumbersome to use than the simple <Array>.contains that it wraps, but rather than returning the boolean result from the function, sends it through an if statement and returns hardcoded true and false values for... Good luck? I guess?

If only we could downvote bad code and make it disappear.

Thoughts and prayer for you

Wait until you see the original multipage version of that method from before the permission system refactoring.
It probably still lurks in the shadows of the (now rarely used) class which has been copied and altered to create the class you are currently looking at...

@Oktokolo this is the earliest version in the current project, but you're right, this is a rewrite of an earlier project bearing the same name

@AlgoRythm
Lucky you - just don't open that forbidden tome of Cthulhu and you are fine...

Pain... At first I though that *maybe* enclosing it in a well named method would at least improve readability... But the way you have to pass the array in anyway... It's awful..

And the if and returning of hardcoded values is outright just dumb...

Java dev don't like C# syntax?

I found this exact same function in a project 2 years ago. Maybe it came from StackOverflow? It was among the nicer parts of the project, I ranted a lot about it back then.

It was a memorable project, my first business experience. After resigning and receiving my salary in cash I found out that the company never existed in public records, and since they told me the client is a massive transnational who wouldn't risk trading with nonexistent companies for such minor investments it's safe to assume the contract and client didn't exist either.

And since the whole team resigned a year after the official deadline, neither did the product, but at this point I'm confident they got "paid" for it regardless.

@homo-lorens It's only a few lines, I'm sure it's original (shit) work

@AlgoRythm I mean it could be part of a class that was copied from SO in whole.

And not even using brackets for the if statement...

Having a function wrapper for that is a good thing than simply using Array method, in case you need to modify the logic.

The hardcoded Boolean would be understandable in JavaScript, but here, it's useless.

@theabbie it's not abstract enough to be modified in the future. It takes a specific type of List<string> as an argument, which would probably need to change if the permission system was actually turned into a legitimate system with permission classes and such.

I mean, maybe. But it's long odds. Everything about this code specifically is shit.

But in general, you're right.

I would introduce an additional framework for this 😅 (keeping it java style)

Using string as the type for a role is the most worrying part for me. It should be a custom type.

@Lensflare
string is a nice type for an ID. And "roles" here are probably just IDs of the actual roles.

Maybe, the business objects are expected to do all the permission checks based on a few fixed user roles.
In that case, there would be no need for an actual role class - it would only consist of the ID.
All the logic would be in the affected objects and their managers.

Jesus Christ! And it's referenced 24 times!

Probably done by someone who at that time did not know you can return a comparison expression. Also, this is nothing compared to the horrendous shit I've seen used in production.

@Oktokolo Strings are bad IDs because it‘s not clear if letter case is ignored or important for equality.
Wrapping them into a custom type with custom equality logic can help with this issue. Many languages have value types (struct) for that so you don‘t lose performance and can also keep value semantics.

The other point is, that even if the roles are ultimately stored as strings in the DB somewhere, doesn’t mean that they also need to be strings in the business domain of the code. You could use enums which are serialized into strings.
It will make clear what roles do exist and make it impossible to make a typo for a role.

Once when I was a kid at a hackathon who thought they were the shit i inverted the password authentication logic and our whole sleep deprived team lost hours... was young enough that even after realizing what I did my ego was still intact

It was the 24 references that really got me
this was really used 24 times? really?

Oh man. I have seen this in a TS project I’m working in. I see things like 'const isNumber=(foo)=> typeof foo === "number"; '

Just check if it is a number! There is no need to wrap it.

@irene I have seen plenty of assertFoo functions which consist of an if statement, and if foo doesn't hold they throw. Apparently it's good practice if you plan to assert the same thing multiple times because you can make the error messages and exception subtypes match.

@irene is there no other way to check the type but to compare it to the name of the type as a string?
In that case, I would actually prefer that isNumber function.

@Lensflare That's a perfectly standard JS type assertion, if you work with it long enough you stop seeing it as a string comparison.

@homo-lorens this is disturbing

@homo-lorens wouldn't startsWith be a bit more performant? Or even just [0] === 'n'

Hard coded string comparisons make me wanna jump out a window. I've used JS for 6+ years now and I still hate checking primitive types. instanceof is much nicer

@AlgoRythm @Lensflare Obviously it isn't an actual string comparison, the engine knows what the possible return values are and it knows what you're comparing with, this will turn into a single byte comparison without even dereferencing the variable (if it's a reference) on all engines made in the last decade.

One of the wonders of JS is that because the simplest tasks can only be expressed in indirect ways, engines are known to optimize very heavily, so if something can be asserted based on the function body you can be sure it'll be optimized at runtime.

On the other hand, thanks to getters and setters and proxy objects, something as simple as writing a value to a key in an object you did not create and reading it out two lines later cannot be optimized.

Basically the fastest way to write JS is to never mutate your parameters.

@cprn If you change the role management system all 24 places will have to be changed anyway because they're all using List<string> and any role management system more developed than this will likely use a type that isn't List<string>. If you were to make an opaque role management system that exposes a `struct RoleSet`, a `struct Role` and a `bool hasRole(RoleSet, Role)` where RoleSet is `{ roles: List<string> }` and Role is `{ role: string }` then it would be modular and you could change the implementations any day. This way it doesn't abstract everything that must be unknown for the operation to be unknown.

Sure, implementation can be replaced with one return line. It has 24 references, though. It's abstraction. Nothing wrong with it. Imagine one day you switch to an external role management system. No need to fix code in all those 24 places just because someone silly made a mistake of using `List.Contains()` directly. Which might return `null` and break function declaration, BTW. Static analysis should catch that.

@cpm How'd devrant swap our comments?f

@homo-lorens Oh yes I am familiar with that “good practice” in parse logic in Express.
The bad side of this is if parsing and validation logic mix you will be throwing randomly and lose control of your application flow in a way that you won’t trace the error. “Does it parse” is a totally different question than “is it valid”. Most times I see those two things squished together and people wonder why the errors aren’t useful.

@homo-lorens that swapping happened to me twice already, no idea why. A bug?

It's still valid to have the call for permissions check abstracted, even if it'll use different types in the future, because it restricts usage to this one abstraction. If you've had 24 calls to `Contains()` that are performed on randomly named `List` variables, e.g. `roles`, `permissions`, `perms`, `caps` or `capabilities`, and check for role identifier string that's stored in a variable that's named in a similar manner, looking for it is hell. While breaking naming convention shouldn't be the case in a sensible codebase, this codebase might not be sensible (guessing by function body here). While unit tests should catch that, it's not guaranteed. Using function as abstraction layer guarantees the compiler will complain about each and every place it's used if you change declaration.

Also, one can hope this list of roles and role identifiers are some kind of enum. Can't always assume the worst.

This sort of code screams "a bloody noob wrote that" and it hurts, especially having had to deal with such people (in JS, Java, C#, ...).

Here's an improvement,

if(roles.Contains(roles) == true)

What the fuckidy fuck is that...

@A4Abhiraj
Another improvement:
return !(!roles.Contains(role));

I don't see what's wrong here. I might be dumber the the dev. Someone should point me in the right direction.

@papsthehacker Not sure if you're serious, but in case you are, in most languages (or possibly all), operations like someArray.contains(something) (or the language equivalent like someArray.includes(something) in JS) return a boolean result (i.e. true or false) so a snippet of code (e.g. a function/method) can simply return it.

But many mongrels like the author of the code in the OP, make it extra verbose like if the expression (`roles.Contains(role)`) didn't return a boolean and the ifs were implicitly casting its output to boolean.

Perfectionism of encapsulation!!

@Berkmann18 Thanks man. Clearer now. The array.contains is gonna return true or false anyway so the if else wrapper and the return values are unnecessarily redundant.

@papsthehacker You're welcome 😁.

I wish the good code (let's call it less bad) in my previous job was like this.

Actually... is this Java? Because according to the official documentation `List<e>.contains()` (although, it's lower letter `contains` so maybe it's not Java after all?) doesn't return `false` so this code makes sense... in very specific context.

https://docs.oracle.com/en/java/...(java.lang.Object)

@cprn it's c#