Pragmatic Refactor: JavaScript Confirmation Messages

Having graduated from school in December, I’ve found a few things were missing from the curriculum. Perhaps the most painful omission was working with legacy code, closely followed by writing tests. I’m hoping to fill in these gaps for others as I continue to learn myself, and I intend these examples based on real-world experience to be a series.

We took some pragmatic steps at the end of last week toward generally improving the code we’ve spent months working on. Cars will serve as the example to convey the real-world refactoring that has taken place.

Our task was to modify the confirmation messages of deleting a car. Cars can currently have four-wheel drive, require diesel fuel, or both. The story added a third attribute to our cars: isHybrid.

First we’ll take a look at what is in place.

var confirmDeletion = function(has4wd, takesDiesel) {
  if (has4wd && takesDiesel)
    return objContentManager.messages.FOUR_WHEEL_DRIVE_AND_DIESEL_DELETE
  if (has4wd)
    return objContentManager.messages.FOURWD_DELETE
  if (takesDiesel)
    return objContentManager.messages.DIESEL_DELETE
  return objContentManager.messages.DEFAULT_DELETE}

Messages are pulled from a resource file, and the constants translate roughly to:

• FOUR_WHEEL_DRIVE_AND_DIESEL_DELETE => “Caution! This will delete a car with -4WD -Diesel Engine Continue?”
• FOURWD_DELETE => “Caution! This will delete a car with -4WD Continue?”
• DIESEL_DELETE => “Caution! This will delete a car with -Diesel Engine Continue?”
• DEFAULT_DELETE => “Caution! This will delete a car Continue?”

Things That Jump Out

WebsiteManager.js

1. The high number of exit points. confirmDeletion is 11 lines long and manages to have four return statements.
2. Surprisingly inconsistent naming of the constants. There is no pattern here and as a result, one cannot reliably guess the names of other constants.
3. No braces on the if statement? No semicolons at the end of lines? This won’t make JSLint happy.
4. We’re dependent on some non-obvious objContentManager global variable.
5. We’re creating a global function.

Messages

1. The four separate messages would make a lot of sense if they were wildly different from each other. Good news — the messages appear to be constructed for string building.
2. The naming of the variables and the alphabetical sorting of the resource file [in Visual Studio] means that we can’t easily see the group of messages that are related to one another.

The Path Forward

We have a team commitment to take whatever code is in place, surround it with tests, and improve it. This was a ripe opportunity for us to keep our promise. At the same time, we have to be mindful of our delivery schedule and not get bogged down in perfection. We’re looking to step in the right direction here, not solve all of its problems.

To not address any of the oddities of the above function would mean introducing several more comparisons and places the function returns, not to mention duplicate resource file messages:

if (has4wd && takesDiesel && isHybrid) return message5
if (takesDiesel && isHybrid) return message6
if (has4wd && isHybrid) return message7
if (isHybrid) return message8

To the functional testing framework! After surrounding this UI-level code with a much-needed functional test, we set out to refactor what was already in place. The idea here is that all tests for existing functionality should be green before and after the refactor.

With our refactoring complete, we now have a function building a message and returning in one place. While not completely DRY, adding our third condition simply required modifying two lines and adding a third conditional.

var confirmDeletion = function(has4wd, takesDiesel, isHybrid) {
  var message = objContentManager.messages.CONFIRM_DELETE_BEGINNING;
  if (has4wd || takesDiesel || isHybrid) {
    message += objContentManager.messages.CONFIRM_DELETE_INTERESTING_ITEM;
    if (has4wd) { message += objContentManager.messages.CONFIRM_DELETE_4WD; }
    if (takesDiesel) { message += objContentManager.messages.CONFIRM_DELETE_DIESEL; }
    if (isHybrid) { message += objContentManager.messages.CONFIRM_DELETE_HYBRID; }
  }
  message += objContentManager.messages.CONFIRM_DELETE_END;
  return message;
}

Note The resource file mentioned sets the JavaScript constants through a master page. It didn’t seem particularly relevant or interesting to include another layer of indirection in this simple refactoring example

Note The INTERESTING_ITEM line had to be added if any of the arguments were true because a sentence precedes the list, but only if the list exists.

Fun Fact This happened on a record 3-story day for my pair!