5 ms·
My cut: const passwordRules = [/[a-z]{1,}/, /[A-Z]{1,}/, /[0-9]{1,}/, /\W{1,}/]; async function createUser(user) { const isUserValid = validat
by johnfn 2y ago
My cut:
const passwordRules = [/[a-z]{1,}/, /[A-Z]{1,}/, /[0-9]{1,}/, /\W{1,}/];
async function createUser(user) {
const isUserValid = validateUserInput(user);
const isPasswordValid = user.password.length >= 8 && passwordRules.every((rule) => rule.test(user.password));
if (!isUserValid) {
throw new Error(ErrorCodes.USER_VALIDATION_FAILED);
}
if (!isPasswordValid) {
throw new Error(ErrorCodes.INVALID_PASSWORD);
}
const userExists = await userService.getUserByEmail(user.email);
if (userExists) {
throw new Error(ErrorCodes.USER_EXISTS);
}
user.password = await hashPassword(user.password);
return userService.create(user);
}
1. Don't use a bunch of tiny functions. This makes it harder for future eng to read the code because they have to keep jumping around the file(s) in order to understand control flow. It's much better to introduce a variable with a clear name.
2. Don't use the `a || throw()` structure. That is not idiomatic JS.
2a. Don't introduce `throwError()`. Again, not idiomatic JS.
3. Use an enum-like object for error codes for clarity.
4. If we must use passwordRules, at least extract it into a global constant. (I don't really like it though; it's a bit too clever. What if you want to enforce a password length minimum? Yes, you could hack a regex for that, but it would be hard to read. Much better would be a list of arrow functions, for instance `(password) => password.length > 8`.
5. Use TypeScript!
- TrianguloY 2y agoMy only nitpick is that the const isPasswordValid = ... should be just before its use (between the first two ifs). Other than that, I prefer this approach (although I would inline the booleans in the ifs to avoid the one-use variables. But that's ok). > Don't use a bunch of tiny functions Exactly this. I only do that when the function is used in more than 10 places and it provides some extra clarity (like something as clamp(minVal,val,maxVal){return max(minVal,min(val,maxVal))} if your language doesn't already have it, of course). I also apply that to variables though, everything that is only used once is inlined unless it really helps (when you create a variable, you need to remember it in case it is used afterwards, which for me is a hard task)
- xigoi 2y ago> My only nitpick is that the const isPasswordValid = ... should be just before its use (between the first two ifs). Wouldn’t that cause the regexes to be recompiled every time you call the function?
- TrianguloY 2y agoI don't think so, if it does it will now too. In fact with my suggestion the regex checks will not run if the user is not valid (as it is now it will always run) const isUserValid = ... if(!isUserValid) ... const isPasswordValid = ... if(!isPasswordValid) ... Etc
- jagged-chisel 2y ago“ Don't use a bunch of tiny functions. This makes it harder for future eng to read the code …” This is where the naming things bit comes in. You name the function correctly, then when the body is read to understand that it works as named, you can remove that cognitive complexity from your brain and continue on. Once you’ve built trust in the codebase that things do what they claim, you can start getting a top-down view of what the code does. That is the power of proper abstraction.
- johnfn 2y agoI don't know. I really don't see any clarity improvements between, `user.password.length >= 8 && passwordRules.every((rule) => rule.test(user.password))` and `validatePassword(password)`. What if you want to add that the password must contain one special character? You don't actually know, by reading the name "validatePassword", if that work has already been done or not. You need to go read the function definition and check. And so even for such a small function, there is no name that you can choose to truly do what you claim and "remove the cognitive complexity from your brain". Once a function gets to a certain threshold of size and reuse, I tend to agree with you. But I think that threshold is quite large - like at least 40-50 lines, or reused at least 3 times.
- jagged-chisel 2y ago`validatePassword` is definitely clearer. When I'm getting familiar with a new codebase, I don't need to know how each thing happens, I need to know what happens. In fact, in your example, you've already abstracted testing against the rules. (AAMOF, the password length requirement should be another rule...) Also with that example, that is about the limit I would tolerate in a conditional that decides the next step in this app. I just need to know "if the password is valid, go to $SCREEN; if not, turn on these messages." I don't care about a string of expressions, I care about what it's doing and I don't want to parse the code every time to figure out "oh, this is just password validation." This is different from verifying that features are implemented - now I need to know the minutia.
- 2y ago
- OptionOfT 2y agoMy issue with this is that you're using exceptions for control flow. A user not being valid is expected (duplicate username). A password not matching a regex is also expected. Then, in general (not seen here as there are no types), I like to use a lot of types in my code. The incoming user would be of type UnvalidatedUser, whereas the return type of this function would be StoredUser or something like that to distinguish the incoming user type with the outgoing. I like to attach semantics to a type, not to conditions: https://existentialtype.wordpress.com/2011/03/15/boolean-blindness/ https://existentialtype.wordpress.com/2011/03/15/boolean-bli...
- johnfn 2y agoThat's a great point, I didn't even think of that. I would use error types as well, yes.
- rerdavies 2y agoMy issue with that is that absolutely NOTHING will ever convince me that returning error codes is a better idea than throwing exceptions. And that you seem to be using 'expected' in some weird cargo-culty sense of the word. An invalid user name is an error, not an expected case.
- nip 2y agoI’ll try: Returning a “Result” with union discrimination on the “success” is far superior to throwing in Typescript in my experience { success: false, reason: “user_already_exists” } | { success: true, data: { user_id: string } - By design you are forced to “deal” with the sad path: in order to get to the data, you must handle the error state - Throwing is not type-safe: you cannot know at build time whether you are handling all thrown errors
- yellowapple 2y ago> An invalid user name is an error, not an expected case. If you ain't expecting users to input bogus data, then you're putting way too much trust in said users. Put simply: is it a bug in your own code if a user tries to use an invalid username? If yes, then throw an exception. If no, then return an error code. Exceptions represent programmer error; error codes represent user error.
- rjbwork 2y ago>1 This is one of my pet peeves. I had an engineer recently wrap Dapper up in a bunch of functions. Like, the whole point of dapper to me is that it gets out of your way and lets you write very simple, declarative SQL database interactions and mapping. When you start wrapping it up in a bunch of function calls, it becomes opaque. DRY has been taken as far too much gospel.
- skydhash 2y agoI always prefer having loadBooks than “select * from books” everywhere. I prefer my code to be just the language version of how I would write an explanation if you ask me one specific question. Not for DRY, but for quick scanning and only diving into details when needed
- rjbwork 2y agoObviously there's a method called LoadBooks. The implementation of LoadBooks is what I'm talking about.
- dclowd9901 2y agoYep, I’d hire you, and not OOP. There was some really questionable code structuring in there and I always wince at abstraction for abstraction’s sake. It always becomes a hindrance when something goes wrong, which is exactly when you don’t want to have a hard time deciphering code.
- ilrwbwrkhv 2y agoSomething about Javascript and Typescript is really ugly to my eyes. I think it is the large keywords at the beginning of every line. Makes it hard to parse and read. I find C++ style much better to read.
- eru 2y ago> 1. Don't use a bunch of tiny functions. This makes it harder for future eng to read the code because they have to keep jumping around the file(s) in order to understand control flow. It's much better to introduce a variable with a clear name. If you have nested functions, that's not a problem. Btw, why do you use regular expressions for some rules, but not for others? Regular expressions are perfectly capable of expressing the length requirement.