Managing network address information

Question

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()]);
}

Answer 1

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.