0
\$\begingroup\$

I would like to know what is the best way of doing the following. I have created an object which makes use of Ajax. It is dependent on the returned data to generate form tags etc. The data is a record which is turned into array for the value I need (NAME/STATUS).

var HTMLFormTag = (function() {

    var selectList;

    var generateSelectHTML = function(data, keySameAsValue, addselectvalue, selectedValue){
        selectList = document.createElement("select");
        selectList.setAttribute("class", "form-control");

        if(addselectvalue){
            var option = document.createElement("option");
            option.setAttribute("value", '');
            option.text = '--select--';
            selectList.appendChild(option);
        }
        $.each(data, function( index, value ) {
            var selected = '';
            var option = document.createElement("option");
            if(selectedValue.length > 0){
                if(value == selectedValue){
                    selected = 'selected';
                    option.setAttribute("selected", selected);
                }else{
                    selected = '';
                }
            }

            if(keySameAsValue){
                option.setAttribute("value", value);
            }else{
                option.setAttribute("value", index);
            }
            option.text = value;
            selectList.appendChild(option);
        });
    }

    return {
        createSelect : function(data,keySameAsValue, addselectvalue, selectedValue){
            generateSelectHTML(data,keySameAsValue, addselectvalue, selectedValue);
            return selectList;
        }
    }
})();


var CommsMatrix = (function() {

    // declare private variables and/or functions
    var statuses = [];
    var loadData = [];
    var protocols = [];

    var generateCMDStatuses = function() {
        loadData.push($.get( "/api/user/list/comms/matrix/data/format/json", {csrf: $.csrfParam('csrf'), select: 'distinct(status)' ,orderby: 'status'}).done(function(data){
            if (data.header.error) {
            } else {
                $.each(data.body.recordset.record, function( index, value ) {
                    if(value.STATUS != "to implement"){
                        statuses.push(value.STATUS);
                    }
                });

            }
        }));
    }

    var generateProtocolList = function() {
        loadData.push($.get( "/api/user/list/sys/list/protocols/format/json", {csrf: $.csrfParam('csrf'), orderby: 'name'}).done(function(data){
            if (data.header.error) {
            } else {
                $.each(data.body.recordset.record, function( index, value ) {
                    protocols.push(value.NAME);
                });

            }
        }));
    }

    return {
        createCM: function(params) {
              createCommsMatrix(params);
            },
        genCMDStatuses: function() {
            generateCMDStatuses();
        },
        genProtocolList: function() {
            generateProtocolList();
        },
        getCMDStatuses: function() {
            return statuses;
        },
        getSupportedProtocols: function() {
            return protocols;
        },
        getLoadData : function(){
            return loadData;
        }
    }

})();

In my main file to get the data I need:

$(function(){
    CommsMatrix.genCMDStatuses();
    CommsMatrix.genProtocolList();
    var loadData = CommsMatrix.getLoadData();
    var selectField = '';
    var selectProtocolField = '';

    t = $('table').DataTable( {
        "paging":   false,
        "ordering": false,
        "info":     false,
        "searching": false,
        }
    } );

    counter = 0;
    $('#addRow').on( 'click', function () {

        var inputSrcField = '<input size="15" name="src['+counter+']" class="form-control"/>';
        var inputDstField = '<input size="15" name="dst['+counter+']" class="form-control"/>';
        var nattype = ['source','destination'];
        var selectNatField = HTMLFormTag.createSelect(nattype, true, true, '');
        selectNatField.setAttribute("name", 'nat['+counter+']');
        selectNatField = selectNatField.outerHTML;

        var inputNatField = '<input size="15" name="nat['+counter+']" class="form-control"/>';

        var inputRemarksField = '<input name="remarks['+counter+']" class="form-control"/>';
        var textareaSrcPort = '<textarea name="srcport['+counter+']">any</textarea>';
        var textareaDstPort = '<textarea name="dstport['+counter+']">any</textarea>';

        t.row.add( [
            selectField,
            inputSrcField,
            '',
            textareaSrcPort,
            inputDstField,
            '',
            textareaDstPort,
            selectNatField,
            inputNatField,
            selectProtocolField,
            inputRemarksField,
        ] ).draw( false );

        counter++;

    } );


    $.when(loadData[0],loadData[1]).done(function(data){
        var statuses = CommsMatrix.getCMDStatuses();
        selectField = HTMLFormTag.createSelect(statuses, true, false, 'To implement');
        selectField.setAttribute("name", 'status['+counter+']');
        selectField = selectField.outerHTML;

        var protocols = CommsMatrix.getSupportedProtocols();
        selectProtocolField = HTMLFormTag.createSelect(protocols, true, false, 'TCP');
        selectProtocolField.setAttribute("name", 'protocol['+counter+']');
        selectProtocolField = selectProtocolField.outerHTML;

        $('#addRow').click();
    });
});

Is this an efficient way to do this?

\$\endgroup\$
  • \$\begingroup\$ Google javascript promises and use them to return your async data.. alternatively you could use callback function.. however those kind of questions go on stack overflow and are off topic for code review. \$\endgroup\$ – I wrestled a bear once. Aug 16 '17 at 13:49
  • \$\begingroup\$ I have updated my question and was told to post it on here stackoverflow.com/questions/45710808/… \$\endgroup\$ – shorif2000 Aug 16 '17 at 14:45
  • 1
    \$\begingroup\$ I'm not sure I understand - does this code work as is or not? If this code is fully functional and you just want to improve it then it belongs here. If it does not work yet it belongs on SO. Some of the StackExchange mods in general have been getting really bad at doing their jobs in the last year or so, so I wouldn't put it past someone to make a mistake. \$\endgroup\$ – I wrestled a bear once. Aug 16 '17 at 15:02
  • \$\begingroup\$ the code works im looking for a way to do it better \$\endgroup\$ – shorif2000 Aug 16 '17 at 15:09
  • \$\begingroup\$ I'm just reading on my phone so I'm not gonna post an answer but I think the biggest improvement you can make is to convert all the native JS stuff to jquery. For example, all the places you're creating elements and then setting attributes and adding to DOM can be done In a single line with jquery. Aside from that your code looks ok to me. Was there something specific you were concerned about? \$\endgroup\$ – I wrestled a bear once. Aug 16 '17 at 16:04
3
\$\begingroup\$
$.get( "/api/user/list/comms/matrix/data/format/json", {csrf: $.csrfParam('csrf'), select: 'distinct(status)' ,orderby: 'status'}).done(function(data){

Use the more standard promise interface that's available for jQuery AJAX functions. Instead of done/fail/success/error, use then(successCb, failCb). Promises allow better flow control for async operations as well.

Even better, you can use the native fetch which is inspired by jQuery's ajax. It also returns a promise, albeit differently. It is always a success, you get the entire response (headers, body, etc.) and you decide what to do with it.

Speaking of promises...

$.when(loadData[0],loadData[1])

Native JS also has Promise.all which is also inspired by jQuery deferreds. It's like $.when except it accepts an array of promises instead of an args list of deferred objects. The resolved value of the promise returned by Promise.all is also an array of values resolved from the promises, in the order they were provided in the input array.

$(function(){
CommsMatrix.genCMDStatuses();
CommsMatrix.genProtocolList();

Instead of storing their deferred objects in an loadData, why not return their deferred objects, then feed them to $.when? Is there really a need to put it inside loadData? In fact, statuses and protocols are also the same thing.

Unless it's a cache of some sort, this intermediary array isn't necessary. Also, I'd leverage HTTP cache if you are doing caching. It's transparent to the code, so you don't do anything special (except for maybe ensuring headers are correct for caching and your request isn't busting the cache).

return {
    createCM: function(params) {
          createCommsMatrix(params);
        },
    genCMDStatuses: function() {
        generateCMDStatuses();
    },
    genProtocolList: function() {
        generateProtocolList();
    },
    getCMDStatuses: function() {
        return statuses;
    },
    getSupportedProtocols: function() {
        return protocols;
    },
    getLoadData : function(){
        return loadData;
    }
}

Whenever I see code like this, I cringe. Often times, this usually just translates to an object of functions. Skip the return {...} and the iife, and just use a straight-up object of functions instead.

    var inputSrcField = '<input size="15" name="src['+counter+']" class="form-control"/>';
    var inputDstField = '<input size="15" name="dst['+counter+']" class="form-control"/>';
    var nattype = ['source','destination'];
    var selectNatField = HTMLFormTag.createSelect(nattype, true, true, '');
    selectNatField.setAttribute("name", 'nat['+counter+']');
    selectNatField = selectNatField.outerHTML;

    var inputNatField = '<input size="15" name="nat['+counter+']" class="form-control"/>';

    var inputRemarksField = '<input name="remarks['+counter+']" class="form-control"/>';
    var textareaSrcPort = '<textarea name="srcport['+counter+']">any</textarea>';
    var textareaDstPort = '<textarea name="dstport['+counter+']">any</textarea>';

You might want to consider using a template library for this, like Mustache.

So for instance, your query code could be simplified into:

const CommsMatrix = {
  getCMDStatuses: function(){
    return fetch('/api/user/list/comms/matrix/data/format/json', {
      body: URLSearchParams({ csrf: $.csrfParam('csrf'), select: 'distinct(status)', orderby: 'status' })
    })
    .then(response => {
      return response.headers.error ? Promise.reject(response.headers.error) : response.json();
    }).then(body => {
      return body.recordset.record.map(v => v.STATUS)
    })
  },
  getSupportedProtocols: function(){
    return fetch('/api/user/list/sys/list/protocols/format/json', {
      body: new URLSearchParams({ csrf: $.csrfParam('csrf'), orderby: 'name' })
    })
    .then(response => {
      return response.headers.error ? Promise.reject(response.headers.error) : response.json();
    }).then(body => {
      return body.recordset.record.map(v => v.NAME)
    })
  }
}

Promise.all([CommsMatrix.getCMDStatuses(), CommsMatrix.getSupportedProtocols()])
  // Destructuring the 2-item array into statuses and protocols variables
  .then(([ statuses, protocols ]) => {
    // You've got statuses and protocols loaded
  })
\$\endgroup\$
  • \$\begingroup\$ Hi, thanks, im getting this error stackoverflow.com/questions/34692554/…. Failed to execute 'fetch' on 'Window': Request with GET/HEAD method cannot have body \$\endgroup\$ – shorif2000 Aug 17 '17 at 9:02
  • \$\begingroup\$ I set the body to null to see how it works but then i get error in Promise.all part SyntaxError: Unexpected token < in JSON at position 0 around .then(([ statuses, protocols ]) => { \$\endgroup\$ – shorif2000 Aug 17 '17 at 9:06
  • \$\begingroup\$ @shorif2000 For the first comment, fetch is fairly new. If you're using an older browser, that might be the cause. Refer to compatibility tables for details. There are also polyfills available in the wild. As for the latter, it's most likely your endpoint responded with HTML when fetch expected JSON. This usually happens when a JSON endpoint responds with an HTML error page. This is something your server should do consistently, and not for the JS to fix. \$\endgroup\$ – Joseph Aug 17 '17 at 12:26
  • \$\begingroup\$ Sorry, had to downside for that one little comment about skipping the anon function and just using an object of functions. That kind of code makes me cringe. Plus the whole purpose of the iife is it allows for "private" methods and variables. You can't just swap one for the other in all cases. \$\endgroup\$ – I wrestled a bear once. Aug 17 '17 at 12:39
  • 1
    \$\begingroup\$ @shorif2000 As mentioned in my previous comment, there is a polyfill for it. And I'm not sure what you mean by "fetch is not an async call but a normal request". fetch is asynchronous. What's probably missing are cookies, which fetch omits and for you to explicitly supply. \$\endgroup\$ – Joseph Aug 18 '17 at 12:13

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.