27
\$\begingroup\$

I'm a developer at a networking company that really has no peers that work above me that I can use for any sort of sounding board for my code, so it's just me. I was wondering if anyone would be willing to take a look at some of my code and just give me a few notes, bullet points, etc... on things that are wrong, things I should look into and so on. If anyone is interested please feel free to comment.

I'm concerned with the JavaScript/jQuery. It seems very coupled and I was wondering if there are any better ways to accomplish the things that I'm doing with this code.

Everything works, but I'm always sure there is room for improvement.

//Global networkInfo Object.  
var networkInfo = {};

$(function () {

    // Bind custom events

    $(networkInfo).bind('updateNetworkInfo', updateNetworkAddress);
    $(networkInfo).bind('updateNetworkRanges', updateNetworkRangeDropDowns);
    $(networkInfo).bind('selectNetworkRanges', selectNetworkRanges);

    // Set form validation

    $('form').validate({
        messages: {
            tbSiteName: "This is an invalid site code"
        }
    });

Add custom rule for site name. This rule has been a pain because this is a webforms application and the name of the input can't be controlled.

    $('#tbSiteName').rules("add", {
        required: true,
        remote: function () {
            var r = {
                url: "/webservices/ipmws.asmx/SiteValid",
                type: "POST",
                data: "{'tbSiteName': '" + $('#tbSiteName').val() + "'}",
                dataType: "json",
                contentType: "application/json; charset=utf-8",
                dataFilter: function (data) { return (JSON.parse(data)).d; }
            }
            return r;
        }
    });

DrillDownProvisioning.aspx doesn't allow the user to change the Subnet mask. They should only be allowed to change this value by clicking on nodes in the DrillDown Tree.

    $('#ddSubnetMask').change(function (e) {
        $(this).val(networkInfo.SubnetMask);
        $('#lblMessageBox')
            .removeClass('hidden')
            .addClass('error')
            .text("Please do not change this value manually");
    });

    // Populate drop down with all the available vlans.

    $.ajax({
        type: 'POST',
        url: '/webservices/ipmws.asmx/GetVlans',
        data: '{}',
        dataType: 'json',
        contentType: 'application/json; charset=utf-8',
        success: function (data) {
            $('#ddNumber').append($('<option />').val("").text(""));
            $.each(data.d, function (index) {
                $('#ddNumber').append($('<option />').val(this.VlanId).text(this.Number));
            });
        }
    });

    // When a vlan number is change auto populate the standard name and description input fields with the predefined values

    $('#ddNumber').change(function () {
        var number = $(this);
        var standardName = $('#tbStandName');
        var description = $('#tbDescription')
        if (number.val() === "") {
            standardName.val("");
            description.val("");
        } else {
            $.ajax({
                type: 'POST',
                url: '/webservices/ipmws.asmx/GetVlanInfo',
                data: "{'number': " + $('#ddNumber').val() + "}",
                success: function (data) {
                    standardName.val(data.d.StandardName);
                    description.val(data.d.StandardName);
                    standardName.valid();
                    description.valid();
                },
                dataType: 'json',
                contentType: 'application/json; charset=utf-8'
            });
        }
    });

    // Populate drop down
    populateSubnetMask();

    // When the network type drop down is changed update the network ranges drop downs.

    $('#ddNetworkTypes').change(function () {
        $(networkInfo).trigger('selectNetworkRanges');
    });

    /*
    * toggle the 6 drop down menus.  If the check box is not marked
    * reset all the values to empty so they don't post.
    */
    $('#enableRange').change(function () {
        if (this.checked) {
            $(networkInfo).trigger('selectNetworkRanges');
        } else {
            $('.mask').val(0);
        }
        $('#networkRangeSelectors').slideToggle();
    });

    /*
    * Open drill down tree for selection
    */
    $('.open').click(function () {
        $('#drilldowntreecontainer').toggle('1000');
        $(this).toggle();
    });

    // Close drill down tree
    $('.close').click(function () {
        $('#drilldowntreecontainer').toggle('1000');
        $('.open').toggle();
    });

Because the page no longer posts back to itself the document.referrer should always be the previous page they visited from. If for some reason a post back is required this will no longer work.

    $('#btnCancel').click(function (e) {
        e.preventDefault();
        window.location.replace(document.referrer);
    });
});

/*
* Populate the dropdown box with the default
* range of 1 through 31
*/
function populateSubnetMask() {
    var dropDown = $('#ddSubnetMask');
    dropDown.append($('<option />').val("").text(""));
    for (var i = 31; i > 0; i--) {
        dropDown.append($('<option />').val(i).text("/" + i));
    }
}

Fetches the predefinied subnet start and stop values from webservice. Populates select boxes with start to stop ranges to be selected.

function updateNetworkRangeDropDowns(e, network, bits) {
    $.ajax({
        type: 'POST',
        url: '/webservices/ipmws.asmx/GetNetworkRanges',
        data: "{'network': '" + network + "', 'bits': " + bits + "}",
        success: function (data) {
            networkInfo.networkRanges = data.d;
            var html = "<option value=''></option>";
            for (var i = data.d.Start; i < data.d.End; i++) {
                html += "<option value='" + i + "'>" + i + "</option>";
            }
            $(".mask").empty().append(html);
            $(networkInfo).trigger('selectNetworkRanges');
        },
        dataType: 'json',
        contentType: 'application/json; charset=utf-8'
    });
}

Looks at the networkInfo.networkRanges object and selects the correct values in the drop down based on the values in the object. Should only select values if the network type is a VLAN(type=9). Will reset all the values to empty if network type is not a VLAN.

function selectNetworkRanges() {
    if ($('#ddNetworkStart option').size() > 0 && $('#ddNetworkTypes').val() == 9) {
        $('#ddNetworkStart').val(this.networkRanges.NetworkStartSelected);
        $('#ddNetworkEnd').val(this.networkRanges.NetworkEndSelected);
        $('#ddFixedStart').val(this.networkRanges.FixedStartSelected);
        $('#ddFixedEnd').val(this.networkRanges.FixedEndSelected);
        $('#ddDHCPStart').val(this.networkRanges.DhcpStartSelected);
        $('#ddDHCPEnd').val(this.networkRanges.DhcpEndSelected);
    } else {
        $('.mask').val(0);
    }
}

Custom event that updates the values in the form inputs for SubnetAddress/SubnetMask with the values in the networkInfo object. Because this function is bound as a custom event you can access the networkInfo object with this

function updateNetworkAddress() {
    $('#tbSubnetAddress').val(this.SubnetAddress);
    $('#ddSubnetMask').val(this.SubnetMask);
    $('#lblMessageBox').addClass('hidden');
    $('#tbSubnetAddress, #ddSubnetMask').valid();
}

This will only be required on the drillDown page, this gets the subnet mask/address from the Radtree on the left and inserts the values into the appropriate form input/select This gets called by DrillDownProvisioning.aspx in the Radtree OnClientNodeClicked="drillDownNodeClick"

function drillDownNodeClick(sender, eventArgs) {
    var node = eventArgs.get_node();
    var address = node.get_value().split("/");
    networkInfo.SubnetAddress = address[0];
    networkInfo.SubnetMask = address[1];
    $(networkInfo).trigger('updateNetworkInfo');
    $(networkInfo).trigger('updateNetworkRanges', [$('#tbSubnetAddress').val(), $('#ddSubnetMask').val()]);
}
\$\endgroup\$

migrated from stackoverflow.com Aug 29 '12 at 12:34

This question came from our site for professional and enthusiast programmers.

  • 4
    \$\begingroup\$ Your code looks very clean and your naming convention is pretty good. The only thing I would prefer to do is to make the code object oriented by using functions as objects. Otherwise, looking at the code without great networking knowledge, it looks very good. \$\endgroup\$ – RobertPitt Dec 27 '10 at 15:55
  • \$\begingroup\$ Couple of questions: 1) Why put populateSubnetMask in to a JS function? I don't see any dependencies off-hand, so why not just hard-code the values? 2) That same control (ddSubnetMask), why not also just disable/readonly it? \$\endgroup\$ – Brad Christie Dec 27 '10 at 15:57
  • \$\begingroup\$ @Brad Christie - 1.) Probably not necessary. 2.) I wasn't aware a select box could be set to readonly? If I set it to disable it wouldn't post back the values when you submit the form. \$\endgroup\$ – Kyle Rogers Dec 27 '10 at 16:01
  • \$\begingroup\$ @KyleRogers: Touché, didn't think about it being a submitted value. And you're right, can't be "readonly" specifically, but more eluding to disabling the element. Everything looks good though from here, without having a live page to test how it plays out on. ;-) \$\endgroup\$ – Brad Christie Dec 27 '10 at 16:06
  • \$\begingroup\$ @KyleRogers, did you ever find a cleaner way to write your code? please post a self answer if you have. \$\endgroup\$ – Malachi Mar 24 '14 at 21:31
19
+100
\$\begingroup\$

From perusing your code a few times:

Custom event abuse

While it's very cool to have custom events, I would simply remove this extra layer of logic. It does not make sense to call $(networkInfo).trigger('selectNetworkRanges'); if you could just call selectNetworkRanges(). I understand that you would loose access to this but you are accessing networkInfo directly in updateNetworkRangeDropDowns anyway.

DRY (Don't Repeat Yourself)

  • In selectNetworkRanges you could do var ranges = this.networkRanges; and then access ranges instead of this.networkRanges every time

  • You are building a dropdown in populateSubnetMask and in updateNetworkRangeDropDowns in a different way even though the functionality is very close. With some deep thoughts you could create a helper function that could build a dropdown for both #ddSubnetMask and .mask

  • $('.open').click and $('.close').click do the same really, you could just do this:

    $('.close,.open').click(function () {
        $('#drilldowntreecontainer').toggle('1000');
        $('.open').toggle();
    });
    

What's in a name?

  • Please avoid short names like r , d

  • It is considered good practice to prefix jQuery results with $ so var $dropDown = $('#ddSubnetMask'); for example

Style

  • Comma separated variables with a single var are considered better so

    var node = eventArgs.get_node(),
        address = node.get_value().split("/");
    

    instead of

    var node = eventArgs.get_node();
    var address = node.get_value().split("/");
    
  • You are using both double quotes and single quotes for your string constants, you should stick single quote string constants. With the possible exception of your data: statements.

Comments

  • Great commenting in general, maybe a tad too verbose at times

  • You should mention in the top that this code relies on the jQuery Validation Plugin, in fact it would have saved time if you mentioned that in your question ;)

Design

  • ddSubnetMask could be set as disabled, you would need a hidden input that contains the actual value to be submitted as per https://stackoverflow.com/a/368834/7602

  • The last few functions starting with populateSubnetMask are not within your $(function () {, I would keep it all together

Magic Numbers

  • You are commenting that VLAN(type=9), I would still advocate to create a var IS_VLAN = 9 and then use that constant

  • Not a magic number per se, 'application/json; charset=utf-8' should be a properly named constant ( it's a DRY issue as well ).

Dancing in the rain

  • Your $.ajax calls should deal with error, it will happen at some point

All in all, I could work with this code. You are correct that the code is tightly coupled. I think that's because of the data you have to work with, so I would not worry about it too much.

\$\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.