1
\$\begingroup\$

I've gotten fed up of writing boilerplate code for managing Ajax calls, and this is the manager class I've come up with

The concept is that it standardizes the interface for RESTful calls, prevents duplicate calls from firing via a request key, also simplifies waiting for a call to complete.

function AjaxManager() {
    this.processes = {};

    this.ajax = (label, uri, settings, success, fail) => {
        console.log({
            ajax: {
                label,
                uri,
                settings
            }
        });
        this.abort(label);
        this.processes[label] = $.ajax(
            uri,
            settings
        ).done((response) => {
            console.log({
                label,
                success: "success",
                response
            });
            if (success) {
                success(response);
            }
        }).fail((response) => {
            console.log({
                label,
                success: "fail",
                response
            });
            if (fail) {
                fail(response);
            }
        }).always(() => {
            console.log("cleanup " + label);
            this.processes[label] = null;
        });
    }
    this.post = (label, uri, data, success, fail) => {
        this.ajax(label, uri, {
            method: "POST",
            data: data
        }, success, fail);
    }
    this.get = (label, uri, data, success, fail) => {
        this.ajax(label, uri, {
            method: "GET",
            data: data
        }, success, fail);
    }
    this.put = (label, uri, data, success, fail) => {
        this.ajax(label, uri, {
            method: "PUT",
            data: JSON.stringify(data),
            contentType: "application/json",
            dataType: "json",

        }, success, fail);
    }
    this.delete = (label, uri, data, success, fail) => {
        this.ajax(label, uri, {
            method: "DELETE",
            data: data
        }, success, fail);
    }
    this.wait = (label, callback) => {
        if (this.processes[label]) {
            this.processes[label].then(callback);
        } else {
            callback();
        }
    }
    this.abort = (label) => {
        if (this.processes[label]) {
            this.processes[label].abort();
        }
    }
}

It looks to be working correctly but I wondered if you guys could give it a once over and check for anything I've missed that needs improving or fixing.

\$\endgroup\$

1 Answer 1

1
\$\begingroup\$
  • Instead of assigning all the methods to this inside one large function, just write a class:

    class AjaxManager {
        constructor() {
            this.processes = {};
        }
        ajax() { ... }
        post() { ... }
    }
    
  • Why do you need the logging? I would just drop all the console.log() calls.

    The required label parameter looks pretty inconvenient. I guess you need to use it like so:

    manager.get("myLabel", "/some/url");
    ...
    manager.abort("myLabel");
    

    Why not simply return the promise itself:

    const request = manager.get("/some/url");
    ...
    request.abort();
    

    This way, you don't need to come up with label names, especially when you never plan to abort. You would also remove the need for a "Manager"... which leads me to my next point.

  • The AjaxManager constructor takes no parameters. So I guess you create it once as a singleton object and use for all requests in your app. Better to avoid such global objects and create one per request.

  • Come to think of it... you probably don't need an object at all. You could replace this all with a single function:

    ajax("POST", "/some/url");
    

    Even the success and fail params could be dropped. You could just use .done() and .fail() methods directly.

So in conclusion... this whole big AjaxManager doesn't really seem to provide many benefits over the jQuery.ajax() API. Perhaps all you need is a simpler helper function to wrap it in.

\$\endgroup\$
4
  • \$\begingroup\$ thanks for your comments, using the old style of class design because of browser comparability, everything i can see says microsoft never added support to IE for the class keyword. the logging is there for testing purposes, once i am happy that the class is working correctly i'll strip it \$\endgroup\$
    – MikeT
    Apr 27, 2018 at 14:22
  • \$\begingroup\$ the promises may be created on different event trigger ie a Leaflet Moveend which if the map has several lays will require one get per layer which i have no guarentee will have completed before the map is moved again, as such i can't be sure the promise will still be in scope which is why they are held in common manager distinguished by a unique key, this allows subsequent calls to abort an incomplete call that was spawned in the previous event trigger \$\endgroup\$
    – MikeT
    Apr 27, 2018 at 14:32
  • \$\begingroup\$ you are quite right about me using it as a quazi singleton, though not globally but per scope so to use the above example of leaflet each map instance would have its own manager. \$\endgroup\$
    – MikeT
    Apr 27, 2018 at 14:43
  • \$\begingroup\$ your comment about returning the promise so that i can use the default done and fail instead of passing in callback functions makes a lot of sense as it reduces the level of complexity nicely thank you \$\endgroup\$
    – MikeT
    Apr 27, 2018 at 14:44

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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