Programming

Figuring out what the code is doing is not the hard part. Documenting the reason you want it to do that (domain knowledge) is the hard part.

Ew no.

Abusing language features like this (boolean expression short circuit) just makes it harder for other people to come and maintain your code.

The function does have opportunity for improvement by checking one thing at a time. This flattens the ifs and changes them into proper sentry clauses. It also opens the door to encapsulating their logic and refactoring this function into a proper validator that can return all the reasons a user is invalid.

Good code is not "elegant" code. It's code that is simple and unsurprising and can be easily understood by a hungover fresh graduate new hire.

Why the password.trim()? Silently removing parts of the password can lead to dangerous bugs and tells me the developer didn't peoperly consider how to sanitize input.

I remember once my password for a particular organization had a space at the end. I could log in to all LDAP-connected applications, except for one that would insist my password was wrong. A trim() or similar was likely the culprit.

In addition to the excellent points made by steventhedev and koper:

user.password = await hashPassword(user.password);

Just this one line of code alone is wrong.

1. It's unclear, but quite likely that the type has changed here. Even in a duck typed language this is hard to manage and often leads to bugs.
2. Even without a type change, you shouldn't reuse an object member like this. Dramatically better to have password and hashed_password so that they never get mixed up. If you don't want the raw password available after this point, zero it out or delete it.
3. All of these style considerations apply 4x as strongly when it's a piece of code that's important to the security of your service, which obviously hashing passwords is.

I would have liked some comments explaining the rules we are trying to enforce or a link to the product requirements for it. Changing the rules requirements is the most likely reason this code will ever be looked at again. The easier you can make it for someone to change them the better. Another reason to need to touch the code is if the user model changes. I suppose we might also want a different password hash or to store the password separately even a different outcome if the validation fails. Or maybe have different ruled for different user types. When building a function like this I think less about "ideals" and more about why someone might need to change what I just did and how can I make it easier for them.

A quick glance and this seemed nothing to do with self documenting code and everything to do with the flaws when code isn't strictly typed.

async function createUser(user) {
    validateUserInput(user) || throwError(err.userValidationFailed);
    isPasswordValid(user.password) || throwError(err.invalidPassword);
    !(await userService.getUserByEmail(user.email)) || throwError(err.userExists);

    user.password = await hashPassword(user.password);
    return userService.create(user);
}

Or

async function createUser(user) {
    return await (new UserService(user))
        .validate()
        .create();
}

// elsewhere…
const UserService = class {
    #user;

    constructor(user) {
        this.user = user;
    }

    async validate() {
        InputValidator.valid(this.user);

       PasswordValidator.valid(this.user.password);

        !(await UserUniqueValidator.valid(this.user.email);

        return this;
    }

    async create() {
        this.user.password = await hashPassword(this.user.password);

        return userService.create(this.user);
    }
}

I would argue that the validate routines be their own classes; ie UserInputValidator, UserPasswordValidator, etc. They should conform to a common interface with a valid() method that throws when invalid. (I’m on mobile and typed enough already).

“Self-documenting” does not mean “write less code”. In fact, it means the opposite; it means be more verbose. The trick is to find that happy balance where you write just enough code to make it clear what’s going on (that does not mean you write long identifier names (e.g., getUserByEmail(email) vs. getUser(email) or better fetchUser(email)).

Be consistent:

1. get* and set* should be reserved for working on an instance of an object
2. is* or has* for Boolean returns
3. Methods/functions are verbs because they are actionable; e.g., fetchUser(), validate(), create()
4. Do not repeat identifiers: e.g., UserService.createUser()
5. Properties/variables are not verbs; they are state: e.g., valid vs isValid
6. Especially for JavaScript, everything is const unless you absolutely have to reassign its direct value; I.e., objects and arrays should be const unless you use the assignment operator after initialization
7. All class methods should be private until it’s needed to be public. It’s easier to make an API public, but near impossible to make it private without compromising backward compatibility.
8. Don’t be afraid to use if {} statements. Short-circuiting is cutesy and all, but it makes code more complex to read.
9. Delineate unrelated code with new lines. What I mean is that jamming all your code together into one block makes it difficult to follow (like run-on sentences or massive walls of text). Use new lines and/or {} to create small groups of related code. You’re not penalized for the white space because it gets compiled away anyway.

There is so much more, but this should be a good primer.