-2
\$\begingroup\$

This code seems a bit long and obtuse for doing something as simple as serializing ajax qet requests. Any ideas on making it more concise?

Pub.ajax = function (config_ajax) {
    var xhr;

    // get
    if (config_ajax.type === 'get') {
        xhr = new win.XMLHttpRequest();
        xhr.open('GET', config_ajax.url, true);
        xhr.onload = function () {
            if (this.status === 200) {
                config_ajax.callback(xhr.responseText);
            }
        };
        xhr.send(null);
    }

    // post
    if (config_ajax.type === 'post') {
        xhr = new win.XMLHttpRequest();
        xhr.open("POST", config_ajax.url, true);
        xhr.setRequestHeader("Content-type",
                "application/x-www-form-urlencoded");
        xhr.onload = function () {
            if (this.status === 200) {
                config_ajax.callback(xhr.responseText);
            }
        };
        xhr.send(config_ajax.data);
    }

    // post for form_data
    if (config_ajax.type === 'multi') {
        xhr = new win.XMLHttpRequest();
        xhr.open("POST", config_ajax.url, true);
        xhr.onload = function () {
            if (this.status === 200) {
                config_ajax.callback(xhr.responseText);
            }
        };
        xhr.send(config_ajax.data);
    }

};

Priv.Queue = (function () {
    var queue = [],
        publik = {};
    function getIndexFromToken(callback) {
        var hold;
        $A.someIndex(queue, function (val, index) {
            if (val.callback === callback) {
                hold = index;
                return index;
            }
        });
        return hold;
    }
    function getBlockedProperty(item) {
        var blocked;
        if (item) {
            blocked = item.blocked;
        } else {
            blocked = false;
        }
        return blocked;
    }
    publik.addItem = function (callback) {
        var temp = {};
        temp.blocked = false;
        temp.callback = callback;
        temp.response_text = null;
        queue.push(temp);
    };
    publik.itemCompleted = function (response_text, callback) {
        var index,
            item,
            blocked;
        index = getIndexFromToken(callback);
        if (index !== 0) {
            queue[index].blocked = true;
            queue[index].response_text = response_text;
        } else {
            item = queue.shift();
            item.callback(response_text);
            blocked = getBlockedProperty(queue[0]);
            while (blocked) {
                item = queue.shift();
                item.callback(item.response_text);
                blocked = getBlockedProperty(queue[0]);
            }
        }
    };
    return publik;
}());

Pub.serialAjax = function (source, callback) {
    Priv.Queue.addItem(callback);
    Pub.ajax({
        type:       'get',
        url:        source,
        callback:   function (response_text) {
            Priv.Queue.itemCompleted(response_text, callback);
        }
    });
};
\$\endgroup\$
1
\$\begingroup\$

Pub.ajax is unnecessary work. You can use the native fetch instead of XHR. If you need to support older browsers, there's a polyfill for it. In addition, async control flow is better done using promises. Again, for older browsers, there is a polyfill for that as well.

With that out of the way, all we're dealing now is that queue. JS arrays are multi-purpose data structures. It can act like a regular array, a stack or a queue among other things. This means implementing an interface for a queue redundant work when a regular array and its methods can do the work.

Another problem with this code is how your queue is tied up with the callback logic. This makes the queue highly untestable. If say I wanted to unit test this queue and check how it works with sample data. How would I make it run without involving the AJAX in the process?

Also, if I understand the code correctly, the queue simply ensures that the callbacks are called in the same sequence they were executed. This is counter-intuitive given that AJAX is supposed to be non-blocking and pseudo-parallel. This hints a design flaw in your architecture. One other flaw is that if the request responds with something other than 200, your queue will be stuck even if the next item is ready for processing.


Anyways, here's my take on it.

let queue = [];

function processQueue(queue){
  // If queue empty or blocked, return what's left.
  if(!queue.length || !queue[0].resolver || !queue[0].data) return queue;

  // Resolve/reject the promise that belongs to the referenced resolver.
  queue[0].resolver.call(null, queue[0].data);

  // Process the items after
  return processQueue(queue.slice(1));
}

function queuedAjax(url){
  return new Promise((resolve, reject) => {

    // Create a queue item and hold a reference in this closure.
    const queueItem = { resolver: null, data: null };

    // Do a request.
    fetch(url).then(res => {

      // Depending on the response status, we assign the promise's
      // resolve/reject into the queue item. We call it there when
      // the item is processed.
      queueItem[resolver] = res.statusText === 200 ? resolve : reject;
      queueItem[data] = res.text();

      // Process the queue, updating it with whatever is left.
      queue = processQueue(queue);
    });

    // Enqueue the item.
    queue.push(queueItem);

  });
}

// Call the custom function and listen for the promise when it comes
// out of the queue.
const promise = queuedAjax('url').then(res => {
  console.log(res)
}, err => {
  console.error(err);
});
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.