3 ms·

“ 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 funct

“ 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 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
- 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.)
dietr1ch 2y ago
Abstraction is way better, I don't really want to know how the password is validated unless I know I'm facing issues with validation (which proper logging tells you about before you even dive into the code). I don't understand why some people prefer being swarmed with details. It's not that they want details, but that they just hate navigating files (layer 8 + tooling problem) or that they "need" to know the details because not knowing them haunts them at night somehow. Also, not having that as a free function makes me think it's not tested at all (although there might be some integration test that hopefully catch all errors at once, but I'm sure they don't either)
- hinkley 2y ago
  It's not abstraction it's separation of concerns. And conflating the two is part of why GP is getting wrapped around the axle here.
hinkley 2y ago
He even knows what the missing function should be called: > isPasswordValid

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
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.)

dietr1ch 2y ago

Abstraction is way better, I don't really want to know how the password is validated unless I know I'm facing issues with validation (which proper logging tells you about before you even dive into the code). I don't understand why some people prefer being swarmed with details. It's not that they want details, but that they just hate navigating files (layer 8 + tooling problem) or that they "need" to know the details because not knowing them haunts them at night somehow. Also, not having that as a free function makes me think it's not tested at all (although there might be some integration test that hopefully catch all errors at once, but I'm sure they don't either)

hinkley 2y ago
It's not abstraction it's separation of concerns. And conflating the two is part of why GP is getting wrapped around the axle here.

hinkley 2y ago

He even knows what the missing function should be called: > isPasswordValid