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));
}

• charlietfl: remove most of the else that do the same thing
• Vivek Parekh: But then how do I check when If conditions are not satisfied?
• charlietfl: can add a variable isValid=false and change that variable along the way. Then just use one toggleClass at the end passing isValid as second argument to determine whether to add or remove class. That would likely remove most of the else

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");
 }
}

• PAEz: OH and do you mean undefined when you say null?....you really should check that. Especially since I changed == to === everywhere.
• Malachi: I know you just made it more readable, but could you ad a little explanation to what all you did?
• PAEz: I basically, um, kinda, um, reversed the else's?..pffft. I went through and recorded the results of the elses first to try and get a clue as to what was going on then re wrote it. Heres the jsbin that I did it in.... jsbin.com/mewax/1/edit ... it originally had your code in the html, but thats gone now. At the top is the working out stuff I did to figure whats going on, maybe that will help you?....sorry, but Im really not sure how to explain it :(

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 ;)

• Malachi: please explain more of what you did in the code please, it makes for a better review, glad to have you here at Code Review!