Self-Documenting Code

It’s not so much indentation that’s an issue, but coupling control flow with errors and exceptions.

Swift does a nice job with `guard` statements that basically bake this in at the language level - a condition succeeds or you must return or throw.

If that control flow is part of business logic, I don’t think there’s any issue with your second example. That’s what it’s there for.

    const passwordRules = [/[a-z]{1,}/, /[A-Z]{1,}/, /[0-9]{1,}/, /\W{1,}/];

    async function createUser(user) {
        const isUserValid = validateUserInput(user);
        const isPasswordValid = user.password.length >= 8 && passwordRules.every((rule) => rule.test(user.password));

        if (!isUserValid) {
            throw new Error(ErrorCodes.USER_VALIDATION_FAILED);
        }


        if (!isPasswordValid) {
            throw new Error(ErrorCodes.INVALID_PASSWORD);
        }

        const userExists = await userService.getUserByEmail(user.email);

        if (userExists) {
            throw new Error(ErrorCodes.USER_EXISTS);
        }

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

2. Don't use the `a || throw()` structure. That is not idiomatic JS.

2a. Don't introduce `throwError()`. Again, not idiomatic JS.

3. Use an enum-like object for error codes for clarity.

4. If we must use passwordRules, at least extract it into a global constant. (I don't really like it though; it's a bit too clever. What if you want to enforce a password length minimum? Yes, you could hack a regex for that, but it would be hard to read. Much better would be a list of arrow functions, for instance `(password) => password.length > 8`.

5. Use TypeScript!

// Creates a user and returns the newly created user's id on success

Hmm, it returns an id? But the @returns is Promise? 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, 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 {
        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);
    }

I have come to appreciate the style of early returns rather than else statements as I have found over the years it generally makes the code easier for me to follow when I’m looking at it possibly years later.

It really depends on the particular condition, but sometimes it just reads better to me to not use the else, and this is because as a style I tend to try have “fail conditions” cause an early return with a success being at the end of the method. But again there are regularly exceptions where trying to do this “just because” would contort the code, so returning an early success result happens often enough.

I have however found that sometimes ReSharper’s “avoid nesting” suggestion (particularly in examples like yours) results in less clear code, but it’s almost always at least not worse and maybe slightly better for the sake of consistency.

EDIT: Having thought about this more, here is why I find early returns generally easier to read than else statements.

With an early return the code is generally more linear to read as when I get to the end of the if block I can instantly see there is nothing else of relevance in the method, I save myself having to needlessly scan for the end of the else block, or even worse, past more code blocks only to find that the rest of the method’s code is irrelevant.

Again, not a hard rule, but a consistent style in a code base also generally makes it easier to read.

> Don't use a bunch of tiny functions

Exactly this. I only do that when the function is used in more than 10 places and it provides some extra clarity (like something as clamp(minVal,val,maxVal){return max(minVal,min(val,maxVal))} if your language doesn't already have it, of course).

I also apply that to variables though, everything that is only used once is inlined unless it really helps (when you create a variable, you need to remember it in case it is used afterwards, which for me is a hard task)