validates_presence_of :purpose

How to refactor a long chunk of asynchronous code is one thing I learned during my Javascript & CouchDB project. It’s not a difficult thing, but I thought it was before I figured out how to do it, so I guess it might be interesting for Javascript newbies.

Asynchronous code

In the first couple of weeks I struggled with this new style of coding before I got the hang of it. If you’re fairly new to Javascript, you’re probably used to just assign a variable in one line and use it in the next line. In the asynchronous world you query for a value and do stuff with it in the callback function of the query.

To illustrate - “normal” code looks like this:

var result = db.query("select * from T");
// use result

Although writing web applications just screams for doing it like this instead:

db.query("select..", function (result) {
// use result
});

(Examples stolen from Ryan Dahl’s Presentation about Node.js.)

The Problem

I have a method that checks if I have to display a notification message. For this it makes a lot of requests to the database, the received data has to be compared with other data, there’s lots of if/else clauses to consider all kinds of conditions and edge cases. It started simple, but then more and more conditions came in, unless I had the much feared diagonal closing bracket line on my screen.

You don’t need to know the details, just look at the picture and you’ll get the impression.

Refactoring

By way of illustration I made up a much shorter example:

checkForConflicts: function(){
  //set arguments
  openDoc(id, function(result){
    // do stuff with arguments
    if(result.bar == whatever){
      // do more stuff with result
      openAnotherDoc(id2, function(result2){
        if(result2.baz == true){
          // show notification message with result2
        }
      });
    }
  });
}

It is time to refactor after you are sure this actually is the way to go and the code basically works (“make it work, then make it pretty”). First you group the code into methods: You split the lines by what they do, and then you name each section. I think around 8 LOC is a good length for a method.

If this was synchronous code, you could just call these methods after each other. But this doesn’t work here, because result and result2 are not available outside the function that retrieves them.

My first approach was to call the functions from within each other. This works - but to see what checkForConflicts does at its very heart, you have to scroll down through all the methods - not much gained:

checkForConflicts: function(){
  //set arguments
  doSomething(arguments);
}

doSomething(arguments){
  openDoc(id, function(result){
    // do stuff with arguments
    if(result.bar = whatever){
      doSomethingElse(result);
    }
  });
}

doSomethingElse(result){
  // do more stuff with result
  openAnotherDoc(id2, function(result2){
    if(result2.baz == true){
      showMessage(result2);
    }
  });
}

showMessage = function(result2){
  // show notification message with result2
}

The way to do it is to use callbacks. Each method gets an anonymous function as (additional) argument: in the method declaration this function has the name callback. This callback is called if all the conditions within the method are met. The callback gets its arguments from within the method that calls it. You have to specify these arguments again in checkForConflicts to have a valid function definition.

checkForConflicts: function(){
  //set arguments
  doSomething(arguments, function(result){
    doSomethingElse(result, function(result2){
      showMessage(result2); 
    });
  });
}

doSomething(arguments, callback){
  openDoc(id, function(result){
    // do stuff with arguments
    if(result.bar = whatever){
      callback(result);
    }
  });
}

doSomethingElse(result, callback){
  // do more stuff with result
  openAnotherDoc(id2, function(result2){
    if(result2.baz == true){
      callback(result2);
    }
  });
}

showMessage = function(result2){
  // show notification message with result2
}

When you look at checkForConflicts, you now immediately see what it does, without being bothered by the details. As a side effect, deciding which code needs access to which arguments helps you to understand your code better. In my case I was able to optimize the use of passed variables a lot.

You might argue that the code is much longer now. First, longer is not worse if it is more readable. Second, this is only an example - if there was code instead of //do stuff the few extra lines wouldn’t carry weight.

Finally here is a screenshot of my refactored code. All the methods don’t fit on one screen, but you can see what checkForConflicts does very quickly.