3
\$\begingroup\$

I want to make this code better. It works now but I think it can be better. I have a working sample on jsfiddle.

When the page loads I have the End IP Address disable until the data is entered. Then I take the values of the input with id attribute txtstartIpAddr and use it as the value for the input with id attribute txtendIpAddr. Any help would be great.

$(document).ready(function () {
  $("#txtstartIpAddr").kendoMaskedTextBox({
      mask: "000.000.000.000",

  });
  $("#txtendIpAddr").kendoMaskedTextBox({
      mask: "000.000.000.000",  
  });
  
  $('#txtstartIpAddr').blur(function() {
    $("#txtendIpAddr").removeAttr('disabled');
    var startIpAddr = $('#txtstartIpAddr').val();
    //console.log(startIpAddr);
    var IPArray = startIpAddr.split('.');
    if (IPArray.length === 4)
    {
      var IPClone = "";
      for(var i = 0; i < 2; i++) {
        IPClone += IPArray[i] + ".";
      }

      $("#txtendIpAddr").val(IPClone);
    }
  });
});
<link href="http://cdn.kendostatic.com/2014.1.318/styles/kendo.default.min.css" rel="stylesheet"/>
<link href="http://cdn.kendostatic.com/2014.1.318/styles/kendo.common.min.css" rel="stylesheet"/>
<script src="https://cdnjs.cloudflare.com/ajax/libs/jquery/3.3.1/jquery.min.js"></script>
<script src="http://cdn.kendostatic.com/2014.1.318/js/kendo.all.min.js"></script>
<div class="form-group">
  <label for="usr">Start IP Address</label>
  <input type="text" class="form-control" id="txtstartIpAddr">
</div>

<div class="form-group">
  <label>End IP Address</label>
  <input type="text" class="form-control" id="txtendIpAddr" disabled=true>
</div>

\$\endgroup\$

1 Answer 1

3
\$\begingroup\$

Overall your code is readable and it is clear to me what you are trying to accomplish by reading the code alone. Your variable names describe their content, with one caveat described below.

Bugs

IP's

The mask restricts which valid ip's you can enter into your textbox, namely only all valid ip's with 3 numbers for each octet. It also allows a wide range of invalid ip's (everything above 255).

Masks on programmatically changed fields

It appears that the library you are using does not work well with input fields that are changed by code. When typing in a field that has been changed by javascript it does not retain the mask, and sometimes add's underscores.

Unlock on invalid ip

You are unlocking the end ip address input even when entering something invalid in the start input box. You are unlocking it even when only clicking in the start input box.

Standards

var, let and const

Virtually every modern browser supports let and const. They superseded var, because their behaviour is more consistent than var.

Variable casing

Follow casing conventions for variables. One such guide can be found on google's github. Local variables are always written in lowerCamelCase. If you see something written in UpperCamelCase it usually denotes a class. Someone reading your code might initially assume IPArray or IPClone to be a class, until they find where it is intialised. Keeping to these conventions makes it easier to read someone's code.

A11y

A11y stands for accessibility. When writing html you should make sure that your application is accessible for people using screenreaders for example.

When using <label> it should be paired with an input field. One way to do this is using the for attribute to pair it with an element. Your first label is paired with an element called usr, which does not exist. To do it correctly, you should put the id of the element you want to link as the value for your for attribute. See the documentation on mdn for more information.

Another way is to surround the form field with the label like so:

<label>
  Your description
  <input type="text">
</label>

By pairing your labels correctly with your form fields you get two benefits. Clicking the label will focus the corresponding form field. Screen readers will use the description of your form field to announce the input field, rather than announcing that there is an input field containing ___.___.___.___.

Alternatives

Validation instead of masks

Masks are somewhat limited by their nature. Consider using validation instead. There are various libraries to do this, but in this case you can even do it all in html if you want. The pattern you need to use becomes a bit convoluted though.

input:valid {
  background: green;
  color: white;
}

input:invalid {
  background: red;
  color: black;
}
<input pattern="(25[0-5]|2[0-4][0-9]|1[0-9][0-9]|[1-9][0-9]|[0-9])\.(25[0-5]|2[0-4][0-9]|1[0-9][0-9]|[1-9][0-9]|[0-9])\.(25[0-5]|2[0-4][0-9]|1[0-9][0-9]|[1-9][0-9]|[0-9])\.(25[0-5]|2[0-4][0-9]|1[0-9][0-9]|[1-9][0-9]|[0-9])">

Array manipulation

Instead of manually concatenating strings, you can use array manipulation to your advantage. If you use .slice(...) (docs) you can keep only part of the array you are still interested in, then join the parts together with a dot.

$(document).ready(function () {
  $("#txtstartIpAddr").kendoMaskedTextBox({
      mask: "000.000.000.000",

  });
  $("#txtendIpAddr").kendoMaskedTextBox({
      mask: "000.000.000.000",  
  });
  
  $('#txtstartIpAddr').blur(function() {
    $("#txtendIpAddr").removeAttr('disabled');
    const startIpAddr = $('#txtstartIpAddr').val();

    const parts = startIpAddr.split('.');
    if (parts.length === 4)
    {
      const clonedIp = `${parts.slice(0, 2).join('.')}.`

      $("#txtendIpAddr").val(clonedIp);
    }
  });
});
<link href="http://cdn.kendostatic.com/2014.1.318/styles/kendo.default.min.css" rel="stylesheet"/>
<link href="http://cdn.kendostatic.com/2014.1.318/styles/kendo.common.min.css" rel="stylesheet"/>
<script src="https://cdnjs.cloudflare.com/ajax/libs/jquery/3.3.1/jquery.min.js"></script>
<script src="http://cdn.kendostatic.com/2014.1.318/js/kendo.all.min.js"></script>
<div class="form-group">
  <label for="usr">Start IP Address</label>
  <input type="text" class="form-control" id="txtstartIpAddr">
</div>

<div class="form-group">
  <label>End IP Address</label>
  <input type="text" class="form-control" id="txtendIpAddr" disabled=true>
</div>

\$\endgroup\$

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