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

• RobertPitt: 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.
• Brad Christie: 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?
• Kyle: @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.
• Brad Christie: @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. ;-)
• Malachi: @KyleRogers, did you ever find a cleaner way to write your code? please post a self answer if you have.

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.