4 ms·

I don't know. I really don't see any clarity improvements between, `user.password.length >= 8 && passwordRules.every((rule) => rule.test(user.password))` and `v

by johnfn 2y ago

I 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.
- strken 2y ago
  It is definitely not clearer to me. I have no idea what happens in the `validatePassword` function. Does it make a synchronous network call that will stop the world for half a second? Does it throw an exception, or return an error object? I will also have to search the rest of the code to see who else calls it, and potentially refactor those callers as well. Any smaller function broken out of a larger function is (slightly) confusing. I find that a genuinely large function is much more confusing than two functions half the size, but a petite little function less than 20 lines long is not confusing to begin with.
  - hinkley 2y ago
    The business rules for passwords and usernames are separate. It's okay for them to be separate methods. You also know that the 'valid password' function is going to list the rules for a valid password. If you get a task to change the password creation rules, do you honestly expect people other than you to remember that code is in the createUser function and not the valid password function?? I don't think you're being honest with yourself about this scenario. Or if you are, this is going to cause you a lot of trouble over time.
    - strken 2y ago
      > If you get a task to change the password creation rules, do you honestly expect people other than you to remember that code is in the createUser function and not the valid password function?? I don't expect anyone to remember where the code is, regardless of which function it's in. I don't even expect them have been aware of every function to begin with. How do you expect people will find the `isPasswordValid` function? Because I know from experience that the way I would find it would likely be by looking at the `createUser` function to see what it does. I might do a case-insensitive grep for the word password first, but even then I'd just have to go look at `createUser` anyway, otherwise I wouldn't know whether `isValidPassword` was dead code or referring to a different password.
      - lurking_swe 2y ago
        how do you unit test this function that’s handling many responsibilities?
        strken 2y ago
        Look at it. It's under 20 lines long and handles one thing, which is creating a user. You unit test it by passing a user with an invalid password. You might possibly decide to put the validation of user input into one function and avoid mocking userService, but I don't see a good justification for splitting password and other user fields into separate functions. I accept that if there was more logic then you might want to split it out, but seriously, it's tiny. The single responsibility principle doesn't justify making your production code much more difficult to read just so the unit tests around it are marginally easier to write.
LorenPechtel 2y ago
I used to feel that way, but over time I've come to realize that for the vast majority of code pulling stuff out so you have only one level of nesting and only one pass/fail decision makes it easier to understand years later. I'm sure the compiler inlines a lot of stuff I pulled out. The exceptions to this are situations where you need to do a bunch of sequential steps and cases where you are looping on stuff in higher dimensions. I have gotten to the point that I consider a comment about the code to be a sign of trouble. (Comments about what the code is interacting with are a different matter.)

by johnfn 2y ago

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.

strken 2y ago
It is definitely not clearer to me. I have no idea what happens in the `validatePassword` function. Does it make a synchronous network call that will stop the world for half a second? Does it throw an exception, or return an error object? I will also have to search the rest of the code to see who else calls it, and potentially refactor those callers as well. Any smaller function broken out of a larger function is (slightly) confusing. I find that a genuinely large function is much more confusing than two functions half the size, but a petite little function less than 20 lines long is not confusing to begin with.
- hinkley 2y ago
  The business rules for passwords and usernames are separate. It's okay for them to be separate methods. You also know that the 'valid password' function is going to list the rules for a valid password. If you get a task to change the password creation rules, do you honestly expect people other than you to remember that code is in the createUser function and not the valid password function?? I don't think you're being honest with yourself about this scenario. Or if you are, this is going to cause you a lot of trouble over time.
  - strken 2y ago
    > If you get a task to change the password creation rules, do you honestly expect people other than you to remember that code is in the createUser function and not the valid password function?? I don't expect anyone to remember where the code is, regardless of which function it's in. I don't even expect them have been aware of every function to begin with. How do you expect people will find the `isPasswordValid` function? Because I know from experience that the way I would find it would likely be by looking at the `createUser` function to see what it does. I might do a case-insensitive grep for the word password first, but even then I'd just have to go look at `createUser` anyway, otherwise I wouldn't know whether `isValidPassword` was dead code or referring to a different password.
    - lurking_swe 2y ago
      how do you unit test this function that’s handling many responsibilities?
      - strken 2y ago
        Look at it. It's under 20 lines long and handles one thing, which is creating a user. You unit test it by passing a user with an invalid password. You might possibly decide to put the validation of user input into one function and avoid mocking userService, but I don't see a good justification for splitting password and other user fields into separate functions. I accept that if there was more logic then you might want to split it out, but seriously, it's tiny. The single responsibility principle doesn't justify making your production code much more difficult to read just so the unit tests around it are marginally easier to write.

LorenPechtel 2y ago

I used to feel that way, but over time I've come to realize that for the vast majority of code pulling stuff out so you have only one level of nesting and only one pass/fail decision makes it easier to understand years later. I'm sure the compiler inlines a lot of stuff I pulled out. The exceptions to this are situations where you need to do a bunch of sequential steps and cases where you are looping on stuff in higher dimensions. I have gotten to the point that I consider a comment about the code to be a sign of trouble. (Comments about what the code is interacting with are a different matter.)