This is our first code-oriented post and discusses some refactoring issues from a Stack Overflow question. Here we’ll take a quick look at the main issues in the code. The screencast goes into more detail about specific refactorings, and refactoring in general–it’s a bit of a slog at 30+ minutes, but particularly for beginners, it may be instructive.

(Caveats: We don’t use jQuery much any more; there’s likely several additional jQuery-specific changes to be made. The refactored code has not been tested, so there may be a few minor gotchas. We’ve assumed no support libraries (e.g., lodash) and there are often some efficiencies there as well.)

Always code as if the guy who ends up maintaining your code will be a violent psychopath who knows where you live.

Here’s the original code as a point of reference. We’ll break down the issues method-by-method and see how we can clean it up and make it easier to understand, test, and think about in the future.

$(function () {
  $('[data-toggle="tooltip"]').tooltip()
})

$(document).on('show.bs.modal', '#BSDOMAddEditEntryModal', function () {
  const $modal = $('#BSDOMAddEditEntryModal')

  if (!$modal.hasClass('show') && !$modal.hasClass('in')) {
    $(".modal-backdrop.fade.in").remove()
    $(".modal-backdrop.fade.show").remove()
  }

  $('.BSDOMInputGrpCalendarIcon').datepicker({
    autoclose: true,
    format:    'mm/dd/yyyy'
  }).on('changeDate', function (e) {
    $(this).parent().find('input:text').val(e.format())
  })
})

function validateBSDOMChkBxL(source, args) {
  const idValue = "[id$='" + source.id + "']"
  args.IsValid = $(idValue).siblings('div').find('table input:checkbox:checked').length > 0
}

function confirmDeleteReportBtn() {
  return confirm("Are you sure you want to delete this record?")
}

function updateUploadStatus(status, message) {
  var $uploadStatusLabel = $("span[id$='lblUploadStatus']")

  $uploadStatusLabel.html(message)

  if (status == "error") {
    $uploadStatusLabel
      .removeClass("BSDOMFileUploadSuccess")
      .addClass("BSDOMFileUploadError invalidMsgCss")
  } else {
    $uploadStatusLabel
      .removeClass("BSDOMFileUploadError invalidMsgCss")
      .addClass("BSDOMFileUploadSuccess")
  }
}

function checkboxErrorVal(source) {
  const idValue   = "[id$='" + source.id + "']"
  const $idObject = $(idValue)

  let displayStyle = "inline"

  if ($idObject.find('input:checkbox:checked').length > 0) {
    displayStyle = "none"
  }

  $idObject.parent().siblings('span').css("display", displayStyle);
}

function BSDOMFileUploadComplete() {
  const id = $("[id$='_DvFileUploadPanel']").attr('id')
  updateUploadStatus("success", "File Uploaded Successfully")

  __doPostBack(id, '')
}

const validFileExts = [ "xls", "xlsx", "pdf", "xml", "doc", "docx", "jpeg", "jpg", "png" ]

const validFileExtMsg = validFileExts.join(", ")

function isFileExtValid(fileExt) {
  return validFileExts.indexOf(fileExt) > -1
}

function createError(name, message) {
  const err   = new Error()
  err.name    = "Upload Error"
  err.message = `* Only accept format in ${validFileExtMsg}`
  return err
}

function BSDOMFileUploadStart(_sender, args) {
  const fileName = args.get_fileName()
  const fileExt  = fileName.substring(fileName.lastIndexOf(".") + 1)

  if (isFileExtValid(fileExt)) {
    return true
  }

  throw createError("Upload Error", `* Only accept format in ${validFileExtMsg}`)
}

function BSDOMFileUploadError(_sender, args) {
  updateUploadStatus("error", args.get_errorMessage())
}

There’s nothing particularly complex about this code, but there are a number of redundancies throughout we thought could be cleaned up.

Duplicate DOM-Ready Handlers

$(document).ready(function () {
  $(function () {
    $('[data-toggle="tooltip"]').tooltip()
  });
});

This runs a DOM-ready block inside another different DOM-ready block; boo-hiss. It’s not going to cause a problem, but it’s redundant and confusing–someone may look at this and think it’s necessary (hint: it’s not). We’ll pull that out:

$(function () {
  $('[data-toggle="tooltip"]').tooltip()
})

Pulling Out jQuery IDs

The validateBSDOMChkBxL method has a dynamic ID in it; we’ll pull that out into a variable. In this case it’s only used once in the method, but if we’re not using string interpolation, it can be confusing to read because of the different quotes:

function validateBSDOMChkBxL(source, args) {
  if ($("[id$='" + source.id + "']").siblings('div').find('table input:checkbox:checked').length > 0) {
    args.IsValid = true;
  } else {
    args.IsValid = false;
  }
}

This turns in to:

function validateBSDOMChkBxL(source, args) {
  const idValue = "[id$='" + source.id + "']"

  if ($(idValue).siblings('div').find('table input:checkbox:checked').length > 0) {
    args.IsValid = true;
  } else {
    args.IsValid = false;
  }
}

If we’re transpiling or targeting a browser(-like) environment that supports string interpolation, we might not pull it out, but if we did, it would be:

  const idValue = `[id$='${source.id}'`

The other potential improvement, since we’re setting a value to true or false based on a logical expression, is to remove the if statement and set it directly:

function validateBSDOMChkBxL(source, args) {
  const idValue = "[id$='" + source.id + "']"
  args.IsValid = $(idValue).siblings('div').find('table input:checkbox:checked').length > 0
}

We’re on the edge on this one because the resulting line is pretty long. Formatting would fix some of that… shrug.

The naming of the method seems off to us, but we’ll leave it alone since we don’t have a complete understanding of the code’s context.

Using ifs To Return the Value of a Logical Expression

Here again we have an unnecessary if statement:

function confirmDeleteReportBtn() {
  if (confirm("Are you sure you want to delete this record?") == true) {
    return true;
  } else {
    return false;
  }
}

We already have a true/false value; returning a different one is redundant:

function confirmDeleteReportBtn() {
  return confirm("Are you sure you want to delete this record?")
}

Pulling Out jQuery Objects and Setting Variables

Here we’re building an ID again, then referring to it multiple times. jQuery caches so there’s no performance penalty, but we believe there’s a readability penalty: we must read the code to understand that we’re operating on the same object throughout. The less reading and thinking we have to do the happier we are.

function CheckboxErrorVal(source) {
  if ($("[id$='" + source.id + "']").find('input:checkbox:checked').length > 0) {
    $("[id$='" + source.id + "']").parent().siblings('span').css("display", "none");
  } else {
    $("[id$='" + source.id + "']").parent().siblings('span').css("display", "inline");
  }
}

The other thing to note is that the only thing this method actually does is set a single CSS value (display) to a string, and that string is determined by how many checkboxes are actually checked (none, or at least one): instead of duplicating the code we’ll just set a variable to "none" or "inline".

We’ll again pull out the constructed ID. We’ll also create a reference to the jQuery object so it’s immediately clear we’re always operating on the same jQuery object. We like to preface jQuery object references with a $ (because that’s a jQuery-ish variable)–this is a matter of preference, but we think it makes it obvious that we (a) don’t need to re-jQuery it by wrapping it in a $(), and (b) that we’re explicitly dealing with things in jQuery-land.

function checkboxErrorVal(source) {
  const idValue   = "[id$='" + source.id + "']"
  const $idObject = $(idValue)

  let displayStyle = "inline"

  if ($idObject.find('input:checkbox:checked').length > 0) {
    displayStyle = "none"
  }

  $idObject.parent().siblings('span').css("display", displayStyle);
}

Isolating Functionality

The only other major irritation with the original code is how it validates the filetypes and reports the error message back. The original code is this:

function BSDOMFileUploadStart(sender, args) {
  var fileName = args.get_fileName();
  var fileExt = fileName.substring(fileName.lastIndexOf(".") + 1);
  if (fileExt == "xls" || fileExt == "xlsx" || fileExt == "pdf" || fileExt == "xml" || fileExt == "doc" || fileExt == "docx" || fileExt == "jpeg" || fileExt == "jpg" || fileExt == "png") {
    return true;
  } else {
    //To cancel the upload, throw an error, it will fire OnClientUploadError
    var err = new Error();
    err.name = "Upload Error";
    err.message = "*Only accept format in xls, xlsx, pdf, doc, docx, jpeg, jpg, png";
    throw (err);

    return false;
  }
}

We have a bunch of file extensions we need to check in a gigantic long line, and an error message if it’s an unacceptable file extension. There are (at least) two problems with this:

• Adding or removing extensions requires changes in two places.
• There’s already a mismatch between acceptable types and the error message.

Did you see the mismatch? We allow XML files, but the error message doesn’t indicate it. So we’ll create a few utility methods and variables to encapsulate this:

const validFileExts = [ "xls", "xlsx", "pdf", "xml", "doc", "docx", "jpeg", "jpg", "png" ]

const validFileExtMsg = validFileExts.join(", ")

function isFileExtValid(fileExt) {
  return validFileExts.indexOf(fileExt) > -1
}

The only other thing we might change is how we construct our Error object: since we’re using a non-standard error property (name) we probably want to have an application-specific Error sub-class, but for now we’ll wrap it up in a simple method:

function createError(name, message) {
  const err   = new Error()
  err.name    = "Upload Error"
  err.message = `* Only accept format in ${validFileExtMsg}`
  return err
}

Now our start-upload method looks like this:

function BSDOMFileUploadStart(_sender, args) {
  const fileName = args.get_fileName()
  const fileExt  = fileName.substring(fileName.lastIndexOf(".") + 1)

  if (isFileExtValid(fileExt)) {
    return true
  }

  throw createError("Upload Error", `* Only accept format in ${validFileExtMsg}`)
}

So What?

We added about ten lines of code to the original codebase. It’s important to understand that refactoring isn’t (necessarily) about reducing the amount of code: it’s about (at the very least):

• Reducing the cognitive load necessary to think about the code.
• Making it easier to change (extend, enhance, modify).
• Making it easier to test.

Our codebase got larger: we’re trading a slightly-larger codebase for code that’s just easier to deal with. Even assuming you (the reader) don’t agree with some of the stylistic choices (or naming, or usefulness of the refactors, etc.) in general it’s easier to read. (And certainly easier to handle acceptable file extension changes!)

Sometimes refactorings will greatly increase the codebase: that’s a tradeoff. We might have more files, more classes, more lines–but if we’re doing it “right” (for various values of “right”) our mainline code will be much easier to reason about, and our codebase will be easier to maintain, extend, and test.