4
\$\begingroup\$

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));
}
\$\endgroup\$
  • \$\begingroup\$ remove most of the else that do the same thing \$\endgroup\$ – charlietfl May 29 '14 at 12:03
  • \$\begingroup\$ But then how do I check when If conditions are not satisfied? \$\endgroup\$ – Vivek Parekh May 29 '14 at 12:14
  • \$\begingroup\$ 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 \$\endgroup\$ – charlietfl May 29 '14 at 14:36
2
\$\begingroup\$

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");
  }
}
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ OH and do you mean undefined when you say null?....you really should check that. Especially since I changed == to === everywhere. \$\endgroup\$ – PAEz May 29 '14 at 15:31
  • \$\begingroup\$ I know you just made it more readable, but could you ad a little explanation to what all you did? \$\endgroup\$ – Malachi May 29 '14 at 16:21
  • \$\begingroup\$ 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 :( \$\endgroup\$ – PAEz May 29 '14 at 20:52
2
\$\begingroup\$

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.

| improve this answer | |
\$\endgroup\$
1
\$\begingroup\$

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

| improve this answer | |
\$\endgroup\$
  • 1
    \$\begingroup\$ 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! \$\endgroup\$ – Malachi Jun 2 '14 at 1:13

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.