Update if (NodeJS callback hell)



  • For a pet project in NodeJS I'm wondering what's the pattern for doing an update of a resource. Basically, I want to first get the resource from the persistence layer and if it exists, update it. Here's the code:

            update: function(id, user, callback) {
                 // First async to get resource
                db.get(id, this.table, function(entity, success){
                    if(success){
                        log.debug('Got the entity');
                        user.id = id;
                        // callback is undefined :-( 
                        db.save(user, this.table, callback);
                    }else{
                        log.info('No entity with ID %s to update', id);
                        // callback is undefined :-(
                        callback(entity, false);
                    }
                });
            },
    

    I have to fix that callback being undefined and also, if there's a better way to handle this sort of operations. I've seen nimble and async libraries so I'm open to using any of those too.


  • Discourse touched me in a no-no place

    Knowing bugger all about nodejs, other than how to not get the capitalisation/punctuation right, isn't this a closure thang?

    Any way of either

    • passing callback into the scope of the db.get(), or
    • passing success out of that scope and use db.save()/callback after calling db.get()

  • sockdevs

    yes, it really does work in both node and the browser. really awesome. look into that ;-)

    as for the other... this should sort out the callback being undefined.

            update: function(id, user, callback) {
                 //ADDED BY accalia
                 callback = callback || function(){};
                 //END ADDED BY accalia
    
                 // First async to get resource
                db.get(id, this.table, function(entity, success){
                    if(success){
                        log.debug('Got the entity');
                        user.id = id;
                        // callback is undefined :-( 
                        db.save(user, this.table, callback);
                    }else{
                        log.info('No entity with ID %s to update', id);
                        // callback is undefined :-(
                        callback(entity, false);
                    }
                });
            },
    


  • 1.

    db.get(id, this.table, function(entity, success){

    This is wrong method signature. Traditionally, the first argument is always an error object or null. The second is the result of operation, if there was no error.

    So the method should look something like this:

    update: function(id, user, callback) {
    	db.get(id, this.table, function (err, entity) {
    		if (err) {
    			// handle error and return
    		}
    
    		// do something with entity
    	}
    }
    

    2.

    To reduce callback hell, use the early exit pattern. So, instead of

    if (success) {
    	// Do a lot of stuff for success
    } else {
    	// Do a few things for failure
    }
    
    if (!success) {
    	// Do a few things for failure	
    	return;
    }
    
    // Do a lot of stuff for success
    

    3.

    I don't understand the part where you update user, instead of loaded entity.

    user.id = id;```

    Shouldn't you update the entity? Or did you just want to check if it existed?

    4.

    callback will be undefined in this case if you didn't provide any. So, in order to have a callback, you need to call your method like this:

    x.update(1, { name: "user name or whatever" }, function (err, res) {
        // I am a callback!
    });
    

    But sometimes, you don't want to give a callback. Sometimes you just want the thing to run on its own, in the "background" so to speak.

    In that case, I use a pattern like this:

    function handleError(err, callback) {
    	if (callback) {
    		callback(err);
    	} else {
    		console.error(err);
    	}
    }
    
    function safe(callback) {
    	if (callback) {
    		callback.apply(this, Array.prototype.slice.call(arguments, 1));
    	}
    }
    
    // ... later
    
    update: function(id, user, callback) {
    	db.get(id, this.table, function (err, entity) {
    		if (err) {
    			return handleError(err, callback);
    		}
    
    		// do something with entity
    
    		return safe(callback, null, { result: true }); // the same as calling "callback(null, {...})"
    	}
    }
    

    In reality, handleError will have a few more bells & whistles, but that's the gist.

    5.

    In this case, you don't need any special library IMO.

    This is a two level nesting operation, so it's pretty clear what's happening just by looking at code.

    The real advantage from things like async is when you need to work with many parallel functions or perform an action on a collection... the kind of stuff that would require a lot of boilerplate otherwise.

    For a linear sequence like this, nested callbacks are fine.



  • Just a head's up: NodeJS' unwritten law of callbacks is to always pass an Error as a callback's first parameter (even if null). So to be more nodejsy your code should be written as:

                db.get(id, this.table, function(err, entity){
                    if(err){
                       // log.error(err.message); // probably done inside db.get()
                        callback(err);
                    }
                    else if(entity){
                        log.debug('Got the entity');
                        user.id = id;
                        // callback is undefined :-( 
                        db.save(user, this.table, callback); // db.save() calls callback(null, ...) on success
                    }
                   else{
                        err = new Error('No entity with ID '+id+' to update');
                        log.info(err.message);
                        callback(err, entity);
                    }
                });
    

    ...and I was Hanzo'd.

    Edit: +1 on the early exit pattern. Just remember to always put a return after your callback calls. :wink:



  • It's dangerous to go alone! Take this:

    var util = require('util');
    
    function CustomError(message){
        if( arguments.length > 1 ){
            message = util.format.apply(null, Array.prototype.slice.call(arguments));
        }
        Error.call(this);
        Error.captureStackTrace(this, CustomError);
        this.name = this.constructor.name;
        this.message = message;
    }
    util.inherits(CustomError, Error);
    
    
    // Use like this:
    
    var obj = { a: 'b', c: {d:'e'} }; // Will be formatted as JSON
    
    var err = new CustomError("Error %d: object %j is %s.", 42, obj, 'stupid');
    require('assert')(err instanceof Error);
    
    throw err;
    


  • Thanks @cartman82 & @accalia


  • sockdevs

    /me bows

    any time! i'm glad to help!



  • @accalia said:

    callback = callback || function(){};

    That is so horribly fucked up. Yes, it's an easy cop out, but it's like using duct tape for building a wardrobe.


  • sockdevs

    @Hanzo said:

    That is so horribly fucked up. Yes, it's an easy cop out, but it's like using duct tape for building a wardrobe.

    welcome to javascript!

    yeah it's weird, but once you get your mind wrapped around how || and && work in JS and the fact that functions are first class citizens (pass them as variables and everything) that actually starts making a lot more sense.

    but yeah, you can do this if you get paid by the lineWant to avoid abusing "boolean" operators:

    if (!callback) {
        callback = function() {};
    }
    

  • sockdevs

    @accalia said:

    @Hanzo said:
    That is so horribly fucked up. Yes, it's an easy cop out, but it's like using duct tape for building a wardrobe.

    welcome to javascript!

    yeah it's weird, but once you get your mind wrapped around how || and && work in JS and the fact that functions are first class citizens (pass them as variables and everything) that actually starts making a lot more sense.


    But then that also leads to continuation passing style, which, if you're not familiar with it, is a great way to make your head spin so fast your ponytails (if you have them) become helicopter blades :smile:



  • sweet race conditions of hell... i was reading this topic, left it to look at a different topic, apparently just as the bus was loading a new post. So now in the list it says I already read this topic but there's a post I missed. :frowning:

    Oh well, at least I can tell that it's got something new. Just the grey bubble is messing with my OCD, those should all be blue only.



  • I didn't do that but changed it to use nested calls.

    IMO it stops the thing from breaking down but it doesn't fix the root of the problem.


Log in to reply
 

Looks like your connection to What the Daily WTF? was lost, please wait while we try to reconnect.