Promise hell, Anti-pattern and Error Handling

Question

I am a newbie in node.js environment. I read a lot of source about implementing Promises and chaining them together. I am trying to avoid anti-pattern implementation but I could not figure it out how can I do it.

There is a User Registration flow in the system. First, I check the username. if there is no user in the DB with this username I create a User model and save it to the DB.

Can you kindly see my comment inline ?

app.js

RegisterUser("existUser","123").then(user=>{
    //send response, etc
}).catch(er => {
    console.log(er);
    //log error,send proper response, etc
    // Should I catch error here or inner scope where RegisterUser implemented ?
});

userService.js

function RegisterUser(username, password) {
    return new Promise((resolve, reject) => {
        GetUser(username) 
            .then(user=>{
                if(user)reject(new Error("User Exists"));
                else{
                    resolve(SaveUser(username,password))// Is that ugly?
                                                        //what if I have one more repository logic here ?
                                                        //callback train... 
                }
            }) 
            .then(user => {
                resolve(user);//If I do not want to resolve anything what is the best practice for it, like void functions?
            }).catch(err=>{
                console.log(err); // I catch the db error what will I do now :)
                reject(err);// It seems not good way to handle it, isn't it ?
                // Upper promise will handle that too. But I dont know how can I deal with that.
            });;
    });
}

repository.js

function GetUser(username) {
    return new Promise((resolve, reject) => {
        if (username === "existUser")
            resolve("existUser");
        else resolve("");
    });
}

function SaveUser(username, password) {
    return new Promise((resolve, reject) => {
        reject(new Error("There is a problem with db"));//assume we forgot to run mongod 
    });
}

The code above seems awful to me. I thought I need to define some method that can chain after GetUser method. like

GetUser(username)
.then(SaveUserRefined)// how could it know other parameters like password, etc
.then(user=> {resolve(user)}) // resolve inside then ? confusing.
.catch(er=>//...);

I feel I do anti-pattern here and create "promise hell" How could a simple flow like that implemented. Validate username and save it.

Thanks.

Answer 1

Your should use async/await syntax to avoid Promise Hell.

Change your code like this

/**
 * This is a promise that resolve a user or reject if GetUser or SaveUser reject
 */
async function RegisterUser (username, password) { 
    var user = await GetUser(username)

    if (user)
        return user;

    var userSaved = await SaveUser(username,password)
    return userSaved
}

If you use RegisterUser inside a async function just code

async function foo () {
    try {
        var usr = await RegisterUser('carlos', 'secret123')
        return usr
    } catch (e) {
        console.error('some error', e)
        return null
    }
}

Or if you use it like a promise

RegisterUser('carlos', 'secret123')
    .then(usr => console.log('goood', usr))
    .catch(e => console.error('some error', e))

Answer 2

If you already are working with promises, then there's no need to create your own. When you call .then on a promise and provide a function saying what to do, that creates a new promise, which will resolve to whatever the function returns.

So your registerUser function should look something like this:

function registerUser(username, password) {
  return getUser(username)
    .then(user => {
      if (user) {
        throw new Error('User Exists');
      }
      return saveUser(username, password)
    });
}

and you use it like this:

registerUser('starfox', 'doABarrelRoll')
  .catch(error => console.log(error);

Note that if SaveUser causes an error, it will end up in this final .catch, since i didn't put any error handling inside registerUser. It's up to you to decide where you want to handle the errors. If it's something recoverable, maybe registerUser can handle it, and the outside world never needs to know. But you're already throwing an error if the user name exists, so the caller will need to be aware of errors anyway.

---

Additionally your getUser and saveUser functions might also be able to avoid creating their own promises, assuming the real implementation calls some function that returns a promise.

Answer 3

Yes, that's the Promise constructor antipattern! Just write

function registerUser(username, password) {
    return getUser(username).then(user => {
        if (user) throw new Error("User Exists");
        else return saveUser(username,password)
    });
}

Notice I also lowercased your function names. You should only capitalise your constructor functions.

Should I catch error here or inner scope where registerUser implemented?

You should catch errors where you can handle them - and you should handle errors at the end of the chain (by logging them, usually). If registerUser doesn't provide a fallback result, it doesn't need to handle anything, and it usually also doesn't need to log the error message on its own (if you want to, see here for an example).

See also Do I always need catch() at the end even if I use a reject callback in all then-ables?.