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.
1 Answer 1
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 ofdate
when you mean the day of the month (e.g.createDays
,days
,#cbDay
,day
), Javascript'sDate#getDate
method is poorly named. Then you can renamedateObj
todate
. - Don't prefix your elements' IDs with
cb
(they're not callbacks). Simply useyear
,month
andday
(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!
-
\$\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 Dixit– Rajesh Dixit2015年11月05日 02:38:17 +00:00Commented 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 meancallback
, although that's not very common either. It's best to avoid abbreviations, except very common ones (e.g.num
fornumber
). 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– Spike2015年11月05日 06:54:41 +00:00Commented Nov 5, 2015 at 6:54