4 ms·
I find the comment at the end interesting // Creates a user and returns the newly created user's id on success Hmm, it returns an id? But the @returns is Prom
by tln 2y ago
I find the comment at the end interesting
// Creates a user and returns the newly created user's id on success
Hmm, it returns an id? But the @returns is Promise<any>? The code as written will change when userService.create changes... without the actual, human readable bit of prose, that potential code issue could be easily overlooked.
Of course, here the code could have a newtype for UserId and return Promise<UserId>, making the code better and then the prose is basically not needed (but please just write a docstring).
FWIW I would document that the `user` parameter is modified. And document the potential race condition between checking the existence of a user and creating a user, and maybe why it was chosen to be done in this order (kinda flimsy in this example). Which would probably lead me to designing around these issues.
Trying to only document via self-documenting code seems to always omit nuances.
/** Create a user and return the id, or throw an error with an appropriate code.
*
* user.password may be changed after this function is called.
*/
async function createUser(user: User): Promise<number> {
if (!validateUserInput(user)) {
throw new Error(err.userValidationFailed);
}
if (isPasswordValid(user.password)) {
// Check now if the user exists, so we can throw an error before hashing the password.
// Note: if a user is created in the short time between this check and the actual creation,
// there could be an unfriendly error
const userExists = !!(await userService.getUserByEmail(user.email));
if (userExists) {
throw new Error(err.userExists);
}
} else {
throw new Error(err.invalidPassword);
}
user.password = await hashPassword(user.password);
return userService.create(user);
}
- Savageman 2y agoThis, but move isPasswordValid below if (!isUserValid)