JavaScript
Article

JavaScript Refactoring Techniques: Specific to Generic Code

By Paul Wilkins

This article was peer reviewed by Dan Prince. Thanks to all of SitePoint’s peer reviewers for making SitePoint content the best it can be!

In a recent thread on SitePoint’s forums, some code was given to let one dropdown box control when another dropdown box is visible. Even though the code worked just fine, I realized that it left much to be desired. It was brittle and incapable of withstanding even small changes to the accompanying HTML.

Here is the original CSS code:

#second { display: none; }
#second.show { display: block; }

and the original JavaScript code:

document.getElementById("location").onchange = function () {
  if (this[this.selectedIndex].value === "loc5") {
    document.getElementById("second").className = "show";
  } else {
    document.getElementById("second").className = "";
  }
};

In this article, I’ll demonstrate some simple JavaScript refactoring techniques that can be applied to the above code in order to make it easier to reuse and more accommodating to future change.

Knowing Which Path to Take

JavaScript has many ways to achieve the same task, and some of them work better than others. Are there ways to improve the code right now so that we don’t have to come back to it later on? Sure! But when there are several possible methods of doing something, how can we determine which one is likely to work best?

One common technique for improving code is to remove duplication (using the don’t repeat yourself principle). From there though, it can be more useful to go from specific to more generic code which allows us to handle a wider range of situations.

Specific code tends to be brittle when it comes to handling future changes. Code doesn’t exist in a vacuum, and will need to change in response to other actions around it and in the HTML code. With the benefit of past experience though, we can look at common changes that occur and improvements that reduce number of times we need to revisit the code. Invariably you will find that this means making the code more generic.

But beware! It can be easy to make our code too generic to the point that it becomes difficult to understand. Striking a good balance between generic and readable is where we find improved code.

JavaScript Refactoring Techniques: Specific to Generic

During the course of test driven development (TDD) you can’t help but to come across this principle as a part of the process:

As the tests get more specific, the code gets more generic.

The Cycles of TDD by Robert C. Martin covers this idea well. The main benefit here is that generic code ends up being able to handle a wider range of situations and scenarios.

Looking at the above code, some obvious specific to generic improvements are immediately available.

  • Storing the strings in variables would help us to manage them from the one place.
  • The onchange event handler is problematic in so far as it can be overwritten. We should consider using addEventListener instead.
  • The className property will overwrite existing class names. We should consider using classList instead.

After making all of these improvements, we’ll end up with code that is more resilient to future changes, and is easier to update. So let’s get started …

Use Variables to Prevent Duplication

The ID of the dropdown box (“location”) and its trigger value (“loc5”) are useful references to keep together. The second <select> element is also being referred to twice, which we can pull out to a separate variable to prevent clutter and provide easier maintenence.

For example, instead of having two references to the same element that would need to be changed if the element’s ID changed:

// bad code
if (...) {
  document.getElementById("second").className = "show";
} else {
  document.getElementById("second").className = "";
}

We can store a reference to this element in a variable, limiting future change to only the place where the variable is assigned:

// good code
var target = document.getElementById("second");
if (...) {
  target.className = "show";
} else {
  target.className = "";
}

By pulling these strings out together to the top of the code, and separating out the parts of the if condition, the specific to generic technique results in code that is easier to maintain, both now and in the future. If any of the identifiers or option values are changed, they can all be easily found in the one place, instead of hunting through the code for all of their occurences.

// improved code
var source = document.getElementById("location");
var target = document.getElementById("second");
var triggerValue = "loc5";

source.onchange = function () {
  var selectedValue = this[this.selectedIndex].value;
  if (selectedValue === triggerValue) {
    target.className = "show";
  } else {
    target.className = "";
  }
};

Improve Event Handling

Traditional event handlers are still quite popular (and have been used correctly in this case), but they have some issues. Chief among them is that when setting an event handler for an element in this way, you will overwrite any previous handler for the same event.

// bad code
source.onchange = function () {
  // ...
};

Currently the above code works. We can demonstrate this using a test.


A quick note about testing

Philosophy: tests are a great way to ensure that the code you write behaves in the way you expect. They reduce the likelihood that changes you make to your code cause something else elsewhere in the code to break. An introduction to testing is unfortunately outside the scope of this article (although SitePoint has a lot of great content on this topic). You will still be able to follow along without having written a test in your life.

Syntax: The following tests use the Jasmine testing framework. Jasmine tests (aka specs) are defined by calling the global Jasmine it function, which takes a string and a further function as arguments. The string is the title of the spec and the function is the spec itself. You can read more about Jasmine on the project’s homepage.


Running the tests

Given the previous state of our code, the following two tests will pass:

it("should add the 'show' class name when the 'loc5' option is selected", function() {
  changeSelectTo(source, "loc5");
  expect(target.classList.contains("show")).toBe(true);
});

it("should remove the 'show' class name when an option value different from 'loc5' is selected", function() {
  changeSelectTo(source, "loc2");
  expect(target.classList.contains("show")).toBe(false);
});

The changeSelectTo function alters the value of the <select> element and the expectation (built using Jasmine’s expect function) ascertains that the element has the correct class name.

But as soon as the onchange handler is altered — which is something any other code is capable of doing — the function that changed the class name is lost and things start to go wrong. We can demonstrate this with a further test:

it("should toggle the class name even when the onchange event is replaced", function () {
  changeSelectTo(source, "loc2");
  expect(target.classList.contains("show")).toBe(false);

  // Overwrite the onchange handler
  source.onchange = function doNothing() { return; };

  changeSelectTo(source, "loc5");
  expect(target.classList.contains("show")).toBe(true); // fails
});

This test fails, as can be seen in this CodePen. Please note that the Jasmine specific code is in a separate Pen, found here.

Refactoring our code to make the test pass

We can easily make this test pass by using addEventListener, which allows any number of functions to be assigned to one event. The false parameter states whether event capture (when true) or event bubbling (when false) is used for the order of events. Quirksmode gives a good overview of the event order for events.

// good code
source.addEventListener("change", function (evt) {
  // ...
}, false);

Here’s how the code is affected by this change:

// improved code
var source = document.getElementById("location");
var target = document.getElementById("second");
var triggerValue = "loc5";

source.addEventListener("change", function () {
  var selectedValue = this[this.selectedIndex].value;
  if (selectedValue === triggerValue) {
    target.className = "show";
  } else {
    target.className = "";
  }
}, false);

With the addEventListener line active all of the tests now pass.

See the Pen mAbzLL by SitePoint (@SitePoint) on CodePen.

Note: in the test code, I’ve named the function toggleShowOnSelectedValue, to make it easier for you to swap between approaches when testing the different onchange techniques:

//source.onchange = toggleShowOnSelectedValue;
source.addEventListener("change", toggleShowOnSelectedValue, false);

Give it a try in the CodePen above. Try toggling the commented out lines and watch what happens.

Improve Class Handling

Another issue with the code is that the second <select> element will lose any previous classes that it might have had, due to className replacing anything that was there before.

// bad code
target.className = "show";

We can see the problem happening in the following failing, which expects a class of indent to still be on the select element after it has been displayed:

it("should retain any existing class names that were on the target element", function () {
  changeSelectTo(source, "loc2");
  target.classList.add("indent");
  expect(target.classList.contains("indent")).toBe(true);

  changeSelectTo(source, "loc5");
  expect(target.classList.contains("indent")).toBe(true); // fails
});

Due to className replacing the entire class name, any other classes that used to be there are removed too.

You can see the failing test in the following CodePen. Please note that the Jasmine specific code is in a separate Pen, found here.

Instead of having these potential problems, we can use classList to add and remove the class names.

// good code
target.classList.add("show");
// ...
target.classList.remove("show");

This now results in the test passing, as can be shown below.:

See the Pen JRPxjG by SitePoint (@SitePoint) on CodePen.

The resulting code after these improvements is now:

// improved code
var source = document.getElementById("location");
var target = document.getElementById("second");
var triggerValue = "loc5";

source.addEventListener("change", function () {
  var selectedValue = this[this.selectedIndex].value;
  if (selectedValue === triggerValue) {
    target.classList.add("show");
  } else {
    target.classList.remove("show");
  }
}, false);

If you are worried about using the classList API as you want to support IE9 and older browsers, you can instead use a separate addClass and removeClass functions to achieve similar results.

Conclusion

Improving your code doesn’t have to be a hard or difficult task.

The specific to generic principle is a beneficial side-effect that comes from test-driven development. Regardless of whether you test code or not though, you too can benefit from these generic code techniques that make your code more flexible. This frees you from returning so often to fix up your code.

Try working some of these improvements in to your own code, and let us know how they improved things for you – or hit us up in the forums for further discussion and assistance.

  • Heggs

    Thanks for the reminders! It’s too easy to sometimes just write JavaScript without thinking about it’s impact on other code. Your examples will make me re-think just writing JS without thinking about if or not I’m doing it the best way!

  • http://rafaelstz.github.io Rafael Corrêa Gomes ♛

    Thanks

  • Vic Vicovich

    I would do that else:
    var show = “show;


    …classList.add(show);

    …classList.remove(show);

    • paulwilkins

      I was getting heat for pulling out the triggerValue string along with the source and target. But yes, extracting out magic values and strings can help to make things easier to manage.

  • raal Bvv

    // What do you think about using the boolean inside the toggle() ?

    source.addEventListener(“change”, function () {
    var selectedValue = this[this.selectedIndex].value;
    target.classList.toggle(“show”, selectedValue === triggerValue);
    }, false);

    • paulwilkins

      Using the second parameter was considered where a separate parameter is used for ease of understanding, but IE11 still has a fair amount of usage and fails to support that second parameter.

  • hayesmaker

    where’s the method `changeSelectTo` defined?

  • nnnnnn321

    Why are people still using this[this.selectedIndex].value to get a select element’s value rather than just this.value? Going via .selectedIndex and the options collection hasn’t been necessary since so many years ago that I can’t even remember when I stopped doing it. Are you trying to support IE4?

    • paulwilkins

      That was going to be another improvement to be made, but became overlooked in the mix. It’s good to note though that improvements don’t just end with the above article.

  • Miszy

    Obviously this code could be improved even further. I’d strongly advise to use “replace temp with query” pattern here :)

    • paulwilkins

      Yes indeed, that would result in the event handler becoming showTarget(target, shouldShowTarget(this.value, triggerValue)); which is a useful refactoring to help make the function clearer to understand.

  • Osvaldo Maria

    I don’t understand how this article is about refactoring technics since it covered a few not so related topics like Tests, classes and event handling

Recommended

Learn Coding Online
Learn Web Development

Start learning web development and design for free with SitePoint Premium!

Get the latest in JavaScript, once a week, for free.