Validating a date in a grid

Question

This function checks if the EndDate of my grid is valid or not. If it is not valid, it turns the bgcolor of the cell to red color with cell-error class.

I need some help in optimizing this block of code. It looks dirty right now.

function onAccrualEndDateChange(current) {
var endDate = $(current).val();
if (endDate != "") {
    var actualEndDate = endDate;
    if (actualEndDate != null) {
        var tempEndDate = parseInt(actualEndDate.getMonth() + 1) + "/" + actualEndDate.getDate() + "/" + actualEndDate.getFullYear();
        if (ValidateDate(tempEndDate)) {
            var startDate = $(current).closest("tr").find("input[id$='StartDate']").val();
            if (startDate != "") {
                var actualStartDate = startDate;
                if (actualStartDate != null) {
                    var tempStartDate = parseInt(actualStartDate.getMonth() + 1) + "/" + actualStartDate.getDate() + "/" + actualStartDate.getFullYear();
                    if (ValidateDate(tempStartDate)) {
                        if (actualStartDate.getDate() > actualEndDate.getDate() || actualStartDate.getMonth() > actualEndDate.getMonth() || actualStartDate.getFullYear() > actualEndDate.getFullYear()) {
                            $(current).closest("td").addClass("cell-error");
                        } else {
                            $(current).closest("td").removeClass("cell-error");
                        }
                    }
                    else {
                        $(current).closest("td").addClass("cell-error");
                    }
                }
                else {
                    $(current).closest("td").addClass("cell-error");
                }
            } else {
                $(current).closest("tr").find("input[id$='StartDate']").closest("td").addClass("cell-error");
            }
        }
        else {
            $(current).closest("td").addClass("cell-error");
        }
    }
    else {
        $(current).closest("td").addClass("cell-error");
    }
} else {
    $(current).closest("td").removeClass("cell-error");
}
}

This is the code to validate the date:

function ValidateDate(date) {
//This validates date with leap year in mm/dd/yyyy or mm/d/yyyy or m/dd/yyyy or m/d/yyyy date format.
var regex = new RegExp("^(((0?[1-9]|1[012])/(0?[1-9]|1\\d|2[0-8])|(0?[13456789]|1[012])/(29|30)|(0?[13578]|1[02])/31)/(19|[2-9]\\d)\\d{2}|0?2/29/((19|[2-9]\\d)(0[48]|[2468][048]|[13579][26])|(([2468][048]|[3579][26])00)))$");

return (date.match(regex));
}

Answer 1

This isn't "optimized", I just tried to make it more readable.
As I cant test it (really wish people would make jsbin examples to test with, buggered if Im going to do it) I have no idea if it works ;)

function onAccrualEndDateChange(current) {
  var endDate = $(current).val();

  if (endDate === "") {
    $(current).closest("td").removeClass("cell-error");
    return;
  }
  if (endDate === null) {
    $(current).closest("td").addClass("cell-error");
    return;
  }

  var actualEndDate = {
    month: endDate.getMonth(),
    date: endDate.getDate(),
    fullYear: endDate.getFullYear()
  };

  var tempEndDate = parseInt(actualEndDate.month + 1, 10) + "/" + actualEndDate.date + "/" + actualEndDate.fullYear;

  if (!ValidateDate(tempEndDate)) {
    $(current).closest("td").addClass("cell-error");
    return;
  }

  var startDate = $(current).closest("tr").find("input[id$='StartDate']").val();

  if (startDate === "") {
    $(current).closest("tr").find("input[id$='StartDate']").closest("td").addClass("cell-error");
    return;
  }
  if (startDate === null) {
    $(current).closest("td").addClass("cell-error");
    return;
  }

  var actualStartDate = {
    month: startDate.getMonth(),
    date: startDate.getDate(),
    fullYear: startDate.getFullYear()
  };

  var tempStartDate = parseInt(actualStartDate.month + 1, 10) + "/" + actualStartDate.date + "/" + actualStartDate.fullYear;

  if (!ValidateDate(tempStartDate)) {
    $(current).closest("td").addClass("cell-error");
    return;
  }

  if (actualStartDate.date > actualEndDate.date || actualStartDate.month > actualEndDate.month || actualStartDate.fullYear > actualEndDate.fullYear) {
    $(current).closest("td").addClass("cell-error");
  } else {
    $(current).closest("td").removeClass("cell-error");
  }
}

Answer 2

Whenever possible, you should fold knowlege into data so programming logic can be stupid and robust (this is the Rule of Representation). Humans are much better at reasoning about more complex data structures than they are at reasoning about complex logic.

You have some confusing conditions that you probably don't want. For example, when startDate is an empty string, you show an error on startInput, but if startDate is null, you show an error on endInput. Maybe that is what you want, but you'd make it easier for a newcomer to your codebase to understand that if you first produce the information you need to render you interface, and then rendered it.

Note: it was my stylistic choice to omit semicolons and rely on automatic semicolon insertion. I find it easier to read.

This function orchestrates the whole operation. It gets the needed elements, clears any showing errors, checks for existing errors, and then shows errors where appropriate.

function onEndDateChange(endInput) {
  var $endInput = $(endInput)
  var $startInput = $endInput.closest("tr").find("input[id$='StartDate']")

  var errors = getErrors($startInput.val(), $endInput.val())

  hideError($endInput)
  hideError($startInput)

  if (errors.noStart || errors.invalidStart) {
    showError($startInput)
  }

  if (errors.noEnd || errors.invalidEnd || errors.anyLarger) {
    showError($endInput)
  }
}

This produces an errors object that is much more communicative than a deep tree of ifs and elses. You may want to design your error conditions differently. For example, it's redundant here to have a noEnd error and an invalidEnd error because an empty end will also fail the invalidEnd test. But, if you want to use that extra information to communicate to the user more about what is wrong, this is where you should add knowledge to the data.

function getErrors (start, end) {
  var start = dateValues(start)
  var end = dateValues(end)

  var errors = {}

  function add (key) {
    errors[key] = true
  }

  if (!end) add('noEnd')
  if (!start) add('noStart')
  if (anyLarger(start, end)) add('anyLarger')
  if (!isValidDate(start)) add('invalidStart')
  if (!isValidDate(end)) add('invalidEnd')

  return errors
}

This elimindates some checking that was repeated frequently. Naming properites month, day, and year makes the boolean questions easier to read.

function dateValues (date) {
  if (!date) return null

  return {
    month: date.getMonth(),
    day: date.getDate(),
    year: date.getYear()
  }
}

This is the same logic you used. I think you might want to check if start.valueOf() is greater than end.valueOf() instead of this. If this is what you meant, here's a clearer way to write it.

function anyLarger (start, end) {
  if (!start || !end) return false
  return start.day > end.day || start.month > end.month || start.year > end.year
}

Again, I moved parts that were getting reused into the only function that was using them. You probably want to make this RegExp more legible by breaking it into more lines and explaining what each branch does. Fat blocks of unexplained logic tend to breed bugs.

function isValidDate(obj) {
  if (!obj) return false

  var date = parseInt(obj.month + 1) + "/" + obj.day + "/" + obj.year

  // validates date with leap year
  // accepts: mm/dd/yyyy mm/d/yyyy m/dd/yyyy m/d/yyyy
  var regex = new RegExp([
    "^(((0?[1-9]|1[012])/(0?[1-9]|1\\d|2[0-8])|(0?[13456789]|1[012])/(29|30)|",
    "(0?[13578]|1[02])/31)/(19|[2-9]\\d)\\d{2}|0?2/29/((19|[2-9]\\d)(0[48]|",
    "[2468][048]|[13579][26])|(([2468][048]|[3579][26])00)))$"
  ].join(''))

  return date.match(regex)
}

You could (and should) swap this simple rendering logic out for a more powerful rendering tool (like mercury or react) when your app needs to do more things than show an error on two rows. As I said before, your life will me much easier if you make logic and rendering really dumb functions. Put the answers to the questions you have in the data. Pass that data to the dumb logic/renderer.

function showError (elem) {
  elem.closest("td").addClass("cell-error");
}

function hideError (elem) {
  elem.closest("td").removeClass("cell-error")
}

If you're looking for another example of folding knowledge into data, I wrote a similar post here.

Answer 3

Don't have enough context to test what you're trying to do but..

function onAccrualEndDateChange(current) {

        // create the jquery object once
        var $ele = $(current),
            $td = $ele.closest("td"),
            $tr = $ele.closest("tr"),
            endDate = $ele.val(),
            endDateObj, startDateObj, $startDate;

        // Empty string - remove error class
        if (endDate === "") return $td.removeClass("cell-error");

        // Falsy value - add error class
        if (! endDate) return $td.addClass("cell-error");

        // Create date object
        // String needs to be formatted acceptable to Date.parse()
        endDateObj = new Date(endDate);

        // Invalid date - add error class
        if (! endDateObj) return $td.addClass("cell-error");

        // Get the start date value
        $startDate = $tr.find("input[id$='StartDate']");

        // Empty string - add error class
        if ($startDate.val() === "") return $startDate.closest("td").addClass("cell-error");

        // Falsy value - add error class
        if (! $startDate.val()) return $td.addClass("cell-error");

        // Start date object
        var startDateObj = new Date($startDate.val());

        // invalid date - - add error class
        if (! startDateObj) return $td.addClass("cell-error");

        // start date greater than end date
        if (startDateObj.getTime() > endDateObj.getTime()){
            return $td.addClass("cell-error");
        }

        // if nothing else - remove error class
        $td.removeClass("cell-error");

}

This was quick and dirty and probably wont work but hopefully it helps. Keep it shallow ;)