I recently had a situation at work wherein a coworker tried to modify a JavaScript function that I had written and ended up introducing some bugs. In reviewing their code, it seemed that their main issue was in not fully understanding what the function was doing. I do, however, believe that it was my fault because the function was — frankly — poorly written to begin with.

Sometimes we have deadlines, and, in order to meet them, we may leave things a mess. I had plans to revisit the code but other tasks took priority. Now that the function was back knocking on my door I saw an opportunity to fix it. Often when we share our code with the world, we share our most meticulously maintained material. But that is not the reality of business all the time. At the end of the day, the product and the customers that use it are the priority and when it comes to deadlines vs perfectly clean code, the deadline wins. However, when we get the chance to go back and clean up after ourselves we should take those opportunities because it’s important that we balance production with our capacity to continue producing.

I’m going to attempt to remedy the diseased function in steps in order to give you an example of how I go through the process of improving code.

The original code

Let’s now look at the original function that gave my fellow developer problems:

function valid(field, visibleField) {
   var state = {
      saved: true,
      requirements: {
         Description: {
            required: true,
            maxlength: 150
         },
         DueDate: {
            date: true
         },
         PriorityID: {},
         TypeID: {}
      }
   };

   if (!state.requirements[field.name]) {
      return true;
   }

   var errorField = visibleField ? visibleField : field;

   // required
   if (state.requirements[field.name].required) {
      if (field.tagName.toLowerCase() == 'input' && field.value.length == 0) {
         errorField.classList.add('inputBorderError');
         return false;
      } else if (field.value === undefined || field.value === '') {
         errorField.classList.add('inputBorderError');
         return false;
      }
   }

   // max length
   if (state.requirements[field.name].maxlength) {
      if (field.value.length > state.requirements[field.name].maxlength) {
         errorField.classList.add('inputBorderError');
         return false;
      }
   }

   // date
   if (state.requirements[field.name].date) {
      if (!moment(field.value, ['MM/DD/YYYY', 'YYYY-M-D'], true).isValid()) {
         errorField.classList.add('inputBorderError');
         return false;
      }
   }

   errorField.classList.remove('inputBorderError');
   return true;
}

Let me also provide some simplified HTML so you can see a sample of the function’s usage:

<form id="myForm">
    <div>
        <input 
            name="Description" 
            type="text" 
            oninput="
                if (valid(this)) { 
                    edit(this); 
                }
            "
        >
    </div>

    <div>
        <input 
            name="DueDate"
            type="text"
            oninput="
                if (valid(this, document.getElementById('myForm'))) { 
                    edit(this); 
                }
            "
        >

    </div>

    <button type="submit">Submit</button>
</form>

The function is decently complex, so let’s go over it to make sure we understand what’s happening. We have a valid() function that takes in the parameters field and visibleField. This is used within the context of an HTML form, so the two parameters are HTML elements. We see a variable immediately declared called state. It has a saved property and a requirements property.

// ...
requirements: {
   Description: {
      required: true,
      maxlength: 150
   },
   // ...
}

One of the immediate issues you may notice is that the saved property in state isn’t even used. Instead of confusing you by explaining its original purpose, let’s just accept that there was a plan for it upon initial development that was since abandoned, making the saved property an artifact of an old design (it never was cleaned out).

// max length
if (state.requirements[field.name].maxlength) {
   if (field.value.length > state.requirements[field.name].maxlength) {
      errorField.classList.add('inputBorderError');
      return false;
   }
}

The keys in the requirements property in the state object are mapped to field names in the form ( Description and DueDate are in our HTML form). The requirements properties’ values, which are objects, map to different validations we want to perform on the field. For example, if we have…

…our max length if-block catches it and returns false if it fails.

We can also see that the function handles displaying the error by adding a class to an element ( errorField.classList.add('inputBorderError')). If a visibleField element is provided, that is what the error is displayed on, otherwise it uses the primary field element.

If the field passes through all of the validation rules that apply to it without returning false, the function eventually returns true, so the function always returns a boolean.

#clean-code #refactoring #javascript #code #code-review