5
\$\begingroup\$

I have some code which works fine. First, it checks that dates format are valid and then, checks that first date is lower than second date. I had to use free input text and custom validator because multiple date formats are supported such as quarter number (2014 Q1) etc.

But I find my code too long for what it's doing. Any advice on code improvement ?

DEMO

function check_handler(){
var from = $('#zeDateFrom').val(), to = $('#zeDateTo').val(); 

if(adu_date_format(from) && adu_date_format(to)){
    if( check_date_period(from.trim(), to.trim()) ){
        alert('OK');
    }
    else alert('Start Date must be lower then End Date.');      
}
else{
    alert('Please use a valid date format. For your information, valid formats are:\ndd/mm/yyyy  (e.g 11/12/2011)\n yyyy  (e.g 2010)\n mm/yyyy  (e.g 11/2009)\n and yyyy qq (e.g 2012 Q2).');
}    

}   

function check_date_period(from, to){  
  var result = false;
  var yearFrom, yearTo, monthFrom, monthTo, dayFrom, dayTo;

  // 0) If something to compare...
  if(from==""||to=="") return true;

  // 1) Test if input is type year only
  if(from.length==4){
      yearFrom = from; monthFrom = 1; dayFrom = 1;
  }
  if(to.length==4){
      yearTo = to; monthTo = 12; dayTo = 0; // 0 for last day of month
  }

  // 2) Test if input is type year[space] quarter

  var mySplit = from.split(" ");
  if(mySplit.length == 2){
    yearFrom = mySplit[0];
    dayFrom = 1;
    if(mySplit[1]=='Q1'){
        monthFrom = 1;
    }
    if(mySplit[1]=='Q2'){
        monthFrom = 4;
    }
    if(mySplit[1]=='Q3'){
        monthFrom = 7;
    }
    if(mySplit[1]=='Q4'){
        monthFrom = 10;
    }
  }
  mySplit = to.split(" ");
  if(mySplit.length == 2){
    yearTo = mySplit[0];
    dayTo = 0;
    if(mySplit[1]=='Q1'){
        monthTo = 3;
    }
    if(mySplit[1]=='Q2'){
        monthTo = 6;
    }
    if(mySplit[1]=='Q3'){
        monthTo = 9;
    }
    if(mySplit[1]=='Q4'){
        monthTo = 12;
    }
  }

  // 3) Test if input is type month/year
  mySplit = from.split("/");
  if(mySplit.length == 2){
    yearFrom = mySplit[1]; monthFrom = mySplit[0]; dayFrom = 1;
  }
  mySplit = to.split("/");
  if(mySplit.length == 2){
    yearTo = mySplit[1]; monthTo = mySplit[0]; dayTo = 0; // 0 for last day of month
  }

  // 4) test if input is type dd/mm/yyyy
  mySplit = from.split("/");
  if(mySplit.length == 3){
    yearFrom = mySplit[2]; monthFrom = mySplit[1]; dayFrom = mySplit[0];
  }
  mySplit = to.split("/");
  if(mySplit.length == 3){
    yearTo = mySplit[2]; monthTo = mySplit[1]; dayTo = mySplit[0];
  }

  // FINALLY: Compare dates
  // Note: 00 is month i.e. January
  monthFrom--;
  if(dayTo!=0) monthTo--;
  var dateOne = new Date(yearFrom, monthFrom, dayFrom); //Year, Month, Day
  var dateTwo = new Date(yearTo, monthTo, dayTo); //Year, Month, Day
  //alert(dayFrom+'/'+(monthFrom-1)+'/'+yearFrom+'---------'+dayTo+'/'+(monthTo-1)+'/'+yearTo)
  //alert(dateOne+'-'+dateTwo);
  if (dateOne < dateTwo) {
    result = true;
  }

  return result;
}


function adu_date_format(text){
  var result = false;

  // 0) If something to test...
  if(text=="") return true;

  // 1) Test if input is type year only
  if(text.length == 4){
    if(text >= 2000 && text <= 2030)
      result = true;
  }

  // 2) Test if input is type year[space] quarter
  var mySplit = text.split(" ");
  if(mySplit.length == 2){
    if((mySplit[0] >= 2000 && mySplit[0] <= 2030) && (mySplit[1].length == 2 && mySplit[1] >= 'Q1' && mySplit[1] <= 'Q4'))
      result = true;
  }

  // 3) Test if input is type month/year
  mySplit = text.split("/");
  if(mySplit.length == 2){
    if((mySplit[0] >= 1 && mySplit[0] <= 12) && (mySplit[1] >= 2000 && mySplit[1] <= 2030))
      result = true;
  }

  // 4) test if input is type dd/mm/yyyy
  mySplit = text.split("/");
  if(mySplit.length == 3){
    if(mySplit[2] >= 2000 && mySplit[2] <= 2030)
      result = true;
  }
  return result;
}
\$\endgroup\$
4
\$\begingroup\$

What jumps me most about this code is that all you do is in 2 functions.

consider moving your "parsing" operations to separate functions, one for validating the format required for parsing and one for the actual parsing:

function isValidQuarterInput(datestring){
   return datestring.split(" ").length == 2;
}

function parseQuarterInputToMonth(datestring){
    var mySplit = datestring.split(" ");
    //move your Q1,Q2,... mapping here
}


then in check_date_period() you can just call the following

if(isValidQuarterInput(from)){monthFrom = parseQuarterInputToMonth(from);}
// this is the alternative ternary operator
monthTo = isValidQuarterInput(to) ? parseQuarterInputToMonth(to) : monthTo;

your var result = false is unnecessary, as you never use it elsewhere, you can just do the following for return:

return dateOne < dateTwo;


I personally would set a boolean wasParsed to be set to true on correct parsing of one input to jump to the new Date() part. This can potentially save execution time, but that's minor. You would have to move the parsing in the following order though:

var wasParsed = false;
if(from.length==4){
  yearFrom = from; monthFrom = 1; dayFrom = 1;
  wasParsed = true;
}

if(!wasParsed){
   if(isValidQuarterInput(from)){
      wasParsed = true;
      monthFrom = parseQuarterInputToMonth(from);
      yearFrom = from.split(" ")[0];
      dayFrom = 1;
   }
}

if(!wasParsed){
  //here is the full date parsing
}
var dateOne = new Date(yearFrom, monthFrom, dayFrom); 
wasParsed = false;
//repeat with to
| improve this answer | |
\$\endgroup\$

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.