3 ms·

`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 happen

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

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.