0

I am trying to test a constructor in node.js that uses asynchronous code, to test out a feature in it. I know the asynchronous code works because I have already ran it, before I put in the feature, as if I were an end-user. In fact, I got it thanks to some user who answered another question I had

The feature

The User constructor has a userRole member, and some async code that checks userType specified against User.userTypes. If userType found, this.userRole is set to userType. Else, exception is thrown.

The code looks like this:

async.forEachSeries(
    User.userTypes,
    function(user_type, callback)
    {
        if (user_type == userType)
        {
            this.userRole = user_type;
            return;
        }
        if (user_type == User.userTypes[User.userTypes.length - 1])
        {
            callback(helpers.invalidData("user_role"));
            return;
        }
        callback(null);
    },
    function(err)
    {
        if (err)
        {
            if (DEBUG)
            {
                console.log("Error from constructor...");
                console.log(JSON.stringify(err, null, '\t') + "\n");
            }
            throw err;
        }
    }
);

The rest of the constructor looks like:

function User(id, email, displayName, password, userType, deleted, cb)
{
    var DEBUG = true;
    var error = null;
    this.userID = id;
    this.email = email;
    this.displayName = displayName;
    this.deleted = deleted;
    var self = this;
    async.forEachSeries(
        User.userTypes,
        function(user_type, callback)
        {
            if (user_type == userType)
            {
                this.userRole = user_type;
                return;
            }
            if (user_type == User.userTypes[User.userTypes.length - 1])
            {
                callback(helpers.invalidData("user_role"));
                return;
            }
            callback(null);
        },
        function(err)
        {
            if (err)
            {
                if (DEBUG)
                {
                    console.log("Error from constructor...");
                    console.log(JSON.stringify(err, null, '\t') + "\n");
                }
                throw err;
            }
        }
    );
    if (User.connectedToDatabase) this._password = password;
    else
    {
        bcrypt.genSalt(10, function (e, salt) {
            bcrypt.hash(password, salt, function (e, hash) {
                if (!e)
                {
                    self._password = hash;
                    if (DEBUG)
                    {
                        console.log("this._password ==" + self._password);
                        console.log("this.userID == " + self.userID);
                    }
                    if (typeof cb === 'function')
                        cb(null, this);

                }
                else
                {
                    console.log("Error occurred: ");
                    console.log(e);
                    if (typeof cb === 'function')
                        cb(e);
                }
            })
        });
    }
}

User.connectedToDatabase = false;
User.BASIC = "basic user";
User.INVENTORY_MANAGEMENT = "inventory";
User.ADMIN = "admin";
User.userTypes = [ User.BASIC, User.INVENTORY_MANAGEMENT, User.ADMIN ];

User.prototype.userID = 0;
User.prototype.email = null;
User.prototype.displayName = null;
User.prototype._password = null;
User.prototype.userRole = User.BASIC;
User.prototype.deleted = false;

User.prototype.responseObject = function() {
    return {
        id: this.userID,
        email: this.email,
        displayName: this.displayName,
        userType: this.userRole
    };
}

My test

I write test() function that takes parameters to pass to User. If User constructed without error, it, along with some of its members are printed to console. Else, error is printed. Here is code:

function test(email, name, pass, type)
{
    try
    {
        var a = new User(Math.round(Math.random() * 32),
            email, 
            name, 
            pass, 
            type
        );
        console.log("Test user created: " + JSON.stringify(a.responseObject(), null, '\t') + "\n");
        console.log("User._password == " + a._password);
        console.log("\n");
    }
    catch (e)
    {
        console.log("User could not be created.\n" + JSON.stringify(e, null, '\t') + "\n");
    }
    /*async.waterfall([
        function(callback){
            var a = new User(Math.round(Math.random * 32),
                email,
                name,
                pass, 
                type);
            callback(null, a);
        },
        function(a, callback) {
            console.log("Test user created: " + JSON.stringify(a, null, '\t') + "\n");
            console.log("User._password == " + a._password);
            console.log("User.userID == " + a.userID);
            console.log("\n");
            callback(null, a);
        }
    ],
    function(err, results)
    {
        console.log("results of test: " + JSON.stringify(results, null, '\t'));
        if (err)
        {
            console.log("User could not be created.\n" + JSON.stringify(err, null, '\t') + "\n");

        }
    })*/

}

My test cases are as follows:

  1. userType matching first element of User.userTypes
  2. userType matching another element of User.userTypes (I picked the last one)
  3. userType not matching any of User.userTypes

At runtime, after new User created, User._password is default value and not the value my asynchronous code comes up with for it.Sample run I suspect I have some async-sync error, but haven't been able to fix it.

Community
  • 1
  • 1
Mike Warren
  • 3,796
  • 5
  • 47
  • 99
  • 1
    **Don't make complex constructors**. It's just asking for (so much) trouble, since you're putting a lot of code into a function the runtime has to call occasionally. If you have a ton of extra work to do, put it in an `init` method of some sort and call that later. – ssube Aug 03 '16 at 19:06
  • @ssube As in call from constructor, or from code that calls constructor? asynchronously or synchronously? – Mike Warren Aug 03 '16 at 19:13
  • 1
    Code in (or called from) the constructor itself. A constructor should take parameters and tuck them away for future requests, but it shouldn't go out over the network and perform a bunch of asynchronous work that could potentially fail later. That is, a constructor does not have a way to indicate failure without throwing, so it should never rely on anything that could fail later. If it does, you can have a partially-constructed object, which is essentially undefined behavior (the worst thing we know of). – ssube Aug 03 '16 at 19:27
  • Doing too much in the constructor is also the most common cause of chicken-and-egg problems (circular dependencies). Doing the heavy, deferred initialization later typically solves that. – ssube Aug 03 '16 at 19:31
  • Can you give example? I am new to this whole async vs. sync concept.... – Mike Warren Aug 03 '16 at 19:33
  • Consider a file reader that takes a path and opens a file. Checking that the path is valid and turning it into a canonical path (getting rid of `..` segments) is quick, well-defined, and synchronous. Doing that in the constructor is a-ok. Opening the file and reading it should probably live elsewhere, because it's complex, may not finish, is asynchronous, and prone to failure. – ssube Aug 03 '16 at 19:36
  • If you'd like to continue talking about this, [let's move it to chat](http://chat.stackoverflow.com/rooms/17)? I'd be happy to explain some of the differences and why I think constructors should be kept simple. – ssube Aug 03 '16 at 19:38
  • [Do not write asynchronous constructors!](http://stackoverflow.com/q/24398699/1048572) – Bergi Aug 03 '16 at 19:52
  • 1
    Actually your code does not work. You'd have to use `self` instead of `this` in the callback. – Bergi Aug 03 '16 at 19:55
  • @ssube: Actually it's not so much about async vs sync, it's about side effects vs. pure computations. A constructor should not affect global state. (It's just that asynchronous functions are typically doing these complex bad things) – Bergi Aug 03 '16 at 19:56
  • @Bergi I fixed it, and now it works. However, I should probably move my async stuff to a method (be that a member of `User` or a method that exists elsewhere). Once I get `connectedToDatabase`, that async stuff won't happen.... – Mike Warren Aug 03 '16 at 21:07

1 Answers1

0

Got it fixed, thanks to @Bergi's advice.

First thing I did: in that async.forEachSeries(), I changed any instance of this to self.

Second thing I did: replaced that asynchronous code (that was working!) to synchronous code. Instead of this:

bcrypt.genSalt(10, function (e, salt) {
        bcrypt.hash(password, salt, function (e, hash) {
            if (!e)
            {
                self._password = hash;
                if (DEBUG)
                {
                    console.log("this._password ==" + self._password);
                    console.log("this.userID == " + self.userID);
                }
                if (typeof cb === 'function')
                    cb(null, this);

            }
            else
            {
                console.log("Error occurred: ");
                console.log(e);
                if (typeof cb === 'function')
                    cb(e);
            }
        })
    });

I simply said: this._password = bcrypt.hashSync(password, bcrypt.genSaltSync(10)); and all was good!

Mike Warren
  • 3,796
  • 5
  • 47
  • 99