3 ms·

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 w

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

by strken 2y ago

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.