7
\$\begingroup\$

I have a date control which has a dropdown for each date, month and year.

The issue is, I can select 31st Nov. 2015 as well, and when I create date object, I get 1st Dec. 2015.

Example

var _date = 31; // value fetched from dropdown
var _month = 10; 
var _year = 2015; 

var date = new Date(_year, _month, _date);
document.write(date)

I want to validate this condition, and my current approach is to match date.getDate() with _date and show an error message if they are different.

Is there a better way to implement this?

Code

var _monthList = [
    {id:"1",name:"January"} ,
    {id:"2",name:"February"} ,
    {id:"3",name:"March"} ,
    {id:"4",name:"April"} ,
    {id:"5",name:"May"} ,
    {id:"6",name:"June"},
    {id:"7",name:"July"} ,
    {id:"8",name:"August"} ,
    {id:"9",name:"September"} ,
    {id:"10",name:"October"} ,
    {id:"11",name:"November"} ,
    {id:"12",name:"December"}];


function createYears(){
	var _year = [];
    for (var i = 2000; i<=new Date().getFullYear(); i++){
    	_year.push({id:i, name:i});
    }
    $("#cbYear").html(createOptions(_year));
}

function createMonths(){
	$("#cbMonth").html(createOptions(_monthList));
}

function createDates(){
	var _dates = [];
    for (var i=1; i<=31; i++){
    	_dates.push({id:i, name:i});
    }
    $("#cbDate").html(createOptions(_dates));
}

function createOptions(dataset){
	var optionStr = "<option></option>";
    $.each(dataset, function(key, value){
        var id = value.id? value.id:key;
        var text = value.name;
        var val = value.value?value.value:value.id;
        optionStr += "<option id="+ id + " name='"+ text+ "' value='"+ val + "'>"+ text + "</option>"
    });
    return optionStr;
}

function registerEvents(){
	$("select").on("change", function(e){
    	validateDate();
    });
}

function validateDate(){
	var _date = $("#cbDate option:selected").val();
    var _month = $("#cbMonth option:selected").val();
    var _year = $("#cbYear option:selected").val();
    if(_year && _month && _date){
        var dateObj = new Date(_year, _month-1, _date);
        if(_date != dateObj.getDate()){
            $("#lblError").text("Please Select a valid date");
        }
        else{
        	$("#lblError").text("");
        }
    }
}
    
(function(){
	createYears();
    createMonths();
    createDates();
    registerEvents();
})()
select{
    background:#fff;
    color:blue;
    width:100px;
    padding:5px;
    border-radius:4px;
    border: 1px solid gray;
}

.error{
    color:red;
}
<script src="https://ajax.googleapis.com/ajax/libs/jquery/1.11.1/jquery.min.js"></script>
<select id="cbYear"></select>
<select id="cbMonth"></select>
<select id="cbDate"></select>
<br />
<span id="lblError" class="error"></span>

Use Case

Select: Invalid Date (e.g. 31st Nov.)

I have also created a JSFiddle for the same.

\$\endgroup\$
0

1 Answer 1

2
\$\begingroup\$

The logic you're using for validating a date looks fine to me.

The code is not bad, but here are a few suggestions for improvement:

IIFE

All of the defined variables and functions leak into global scope. Put them all inside the IIFE to avoid the scope leak: (Note the trailing semicolon)

(function(){
    var _monthList = ...
    function createYears() {
        ...
    }

    ...

    createYears();
    createMonths();
    createDates();
    registerEvents();
})();

And it's better to execute this function only when the document is ready, so it works regardless of when the script is loaded:

$(function() {
    ...
});

Naming

  • Don't prefix your variable names with an underscore. This convention is only used for private properties of objects, which you don't have in your code.
  • Use day instead of date when you mean the day of the month (e.g. createDays, days, #cbDay, day), Javascript's Date#getDate method is poorly named. Then you can rename dateObj to date.
  • Don't prefix your elements' IDs with cb (they're not callbacks). Simply use year, month and day (element IDs don't clash with variable/function names, so they don't need to be prefixed).
  • lblError -> errorLabel. No need to abbreviate.
  • dataset -> options, to be more descriptive.

Make each function do one thing

createYears both generates the year array and manipulates the DOM. Generate the array outside the function instead: (like you did for createMonths)

var yearList = (function() {
    var years = [];
    for (var i = 2000; i<=new Date().getFullYear(); i++){
        years.push({id:i, name:i});
    }
    return years;
})();

Do the same for createDays.

validateDate both checks if the date is valid and updates #errorLabel accordingly. Use a seperate function for testing if a date is valid:

function isDateValid(day, month, year) {
    var date = new Date(year, month-1, day);
    return day == date.getDate();
}

createOptions

Every time createOptions is called, the return value is set as the HTML value of an element. It's better to pass the element to createOptions as an argument and let it manipulate the element to eliminate this repetition.

Each option passed to this function is known to have id and name properties, so you don't need to check which properties are present.

You don't need to set the IDs for the options, it's enough to set the values. This means the id property of the options in the array argument should be renamed to value (Change id to value when generating the arrays).

Finally, you can use jQuery for creating the elements:

function createOptions(select, options) {
    options.forEach(function (option) {
        select.append($('<option>', { text: option.name, value: option.value }));
    });
}

Now createYears looks like this:

function createYears() {
    createOptions($('#year'), yearList);
}

Which is so short that there is no need to make createYears, createMonths and createDays multiple functions.

Seperating logic from DOM

Put all the functions that are responsible for the logic first, and after them put code that accesses the page's DOM: (i.e. code that has strings with specific element IDs)

var dayList = ...;
var monthList = ...;
var yearList = ...;

function createOptions(select, options){
    ...
}

function isDateValid(day, month, year) {
    ...
}

createOptions($('#year'), yearList);
createOptions($('#month'), monthList);
createOptions($('#day'), dayList);

$("select").on("change", function validateDate() {
    var day = $("#day").val(); /* NOTE: No need to use additional selectors when you're already selecting by ID */
    var month = $("#month").val();
    var year = $("#year").val();
    if(year && month && day) {
        /* NOTE: I used a ternary expression to shorten the code */
        $("#errorLabel").text(isDateValid(day, month, year) ? "" : "Please select a valid date");
    }
});

This makes the code easier to understand and more reusable (e.g. it will make having multiple date forms on the same page easier).

Good luck!

\$\endgroup\$
2
  • \$\begingroup\$ thanks for your feedback. Will incorporate it in my code. Also I used cb in html as a prefix for combobox. Is it bad to use such prefix? Like 'lbl' for label, 'txt' for text etc. \$\endgroup\$
    – Rajesh
    Nov 5, 2015 at 2:38
  • 1
    \$\begingroup\$ @Rajesh cb isn't often use to mean combobox, so it'll probably not be understood by someone who reads the code (e.g. me :-)). It's sometime used to mean callback, although that's not very common either. It's best to avoid abbreviations, except very common ones (e.g. num for number). But as I said there's no need to prefix the element IDs. Instead, you may want to use the more descriptive selector $('select#year') in your code so it's obvious it's a <select> element. \$\endgroup\$
    – Spike
    Nov 5, 2015 at 6:54

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.