-2

I'm creating a function to compare a custom date-time with current date-time. I convert a string dd/MM/yyyy HH:mm:ss to new Date() before comparing.

Here is the code:

var getCurrentDateTime = function () {
 var dt = new Date(),
  dd = dt.getDate(),
  MM = dt.getMonth() + 1,
  yyyy = dt.getFullYear(),
  HH = dt.getHours(),
  mm = dt.getMinutes(),
  ss = dt.getSeconds();

 return new Date(yyyy, MM, dd, HH, mm, ss)
};

var parseTimeString = function (d) { 
 
        // `d` formatting: 'dd/MM/yyyy HH:mm:ss'

 var d_d = d.split(' ')[0],
         d_t = d.split(' ')[1],
                //x = new Date(2016, 01, 14, 21, 40, 00),
  x = new Date(+d_d.split('/')[2], +d_d.split('/')[1] - 1, 
                             +d_d.split('/')[0], +d_t.split(':')[0], 
                             +d_t.split(':')[1], +d_t.split(':')[2]),
  c = getCurrentDateTime(),
  z = Math.abs((c.getTime() - x.getTime())/1000);  

 if (z <= 29) {
  return 'Just now'
 }
 if (z > 29 && z < 60) {
  return '30 seconds ago'
 }
 if (z >= 60 && z < 120) {
  return '1 minute ago'
 }
 if (z >= 120 && z < 3600) {
  return (c.getMinutes() - x.getMinutes()) + ' minutes ago'
 }
 if (z >= 3600 && z < 7200) {
  return '1 hour ago'
 }
 if (z >= 7200 && z < 86400) {
  return (c.getHours() - x.getHours()) + ' hours ago'
 }
 if (z >= 86400 && z < 172800) {
  var m = x.getMinutes();
  return 'Yesterday ' + x.getHours() + ':' + (m < 10 ? '0' + m : m)
 }
 if (z >= 172800) {
  var dd = x.getDate(),
   MM = x.getMonth() + 1,
   yyyy = x.getFullYear(),
   m = x.getMinutes();
  dd = dd < 10 ? '0' + dd : dd;
  MM = MM < 10 ? '0' + MM : MM;
  return dd + '/' + MM + '/' + yyyy + ' at ' + x.getHours() + ':' + (m < 10 ? '0' + m : m)
 }
};

$('button').click(function () {
  setInterval(function () {
    var x = parseTimeString('14/01/2016 21:40:00');
    $('body').html($('<p>').text(x))
  }, 1000)
})
<script src="https://ajax.googleapis.com/ajax/libs/jquery/2.1.1/jquery.min.js"></script>
<button>Click me</button>

My problem:

The line

x = new Date(+d_d.split('/')[2], +d_d.split('/')[1] - 1, 
             +d_d.split('/')[0], +d_t.split(':')[0], 
             +d_t.split(':')[1], +d_t.split(':')[2])

is not converted to new Date() correctly. Current date-time is: 2016/01/14 21:40:00, but it printed 14/01/2016 at 21:40 instead of Just now

To check again, I've replaced that line to

x = new Date(2016, 01, 14, 21, 40, 00)

and it's working perfectly. So, why?

p/s: And my sub-question: Is there any problem if I use more than 20 intervals in the same time? (Does my web page run slowly?)

Tân
  • 1
  • 15
  • 56
  • 102

3 Answers3

2

Firstly, as I pointed out in the comments, your getCurrentDateTime() function is over-complicated, and also has an "off by one" bug in the month field, which is probably responsible for your actual problem:

function getCurrentDateTime() {
    return new Date();
}

That function is now arguably so trivial that it's not worth having.

Secondly, you should split the parsing of the date from the subsequent bits that turn it into something human readable:

function parseDateTime(s) {
    var date_time = s.split(' ');
    var date = date_time[0];
    var time = date_time[1];
    var dmy = date.split('/').map(Number);
    var hms = time.split(':').map(Number);

    return new Date(dmy[2], dmy[1] - 1, dmy[0], hms[0], hms[1], hms[2]);
}

or if you like ES6 Code Golf:

let parseTime=(s)=>new(Function.prototype.bind.apply(Date,
         s.match(/^(\d\d?)\/(\d\d?)\/(\d{1,4})\s+(\d\d?):(\d\d?):(\d\d?)$/)
          .map((_,i,a)=>a[i<4?4-i:i]-+(i===2))))

and then:

//
// pass an already-parsed `Date` object here
//
function longAgo(t) {

    // no need for conversions - subtraction will automatically
    // call `.getValue()` to get the milliseconds value
    //
    // - also no call to 'Math.abs' so that the function works
    //   correctly for future dates
    var z = (Date.now() - t) / 1000;

    if (z >= 0 && z < 30) {
        // etc
    }
}
Alnitak
  • 334,560
  • 70
  • 407
  • 495
  • I guess you're avare that the variable names don't really match in the `parseDate` function... – Tomáš Zato Jan 14 '16 at 15:23
  • @HappyCoding the important thing here is the separation of logic - break your code into smaller chunks that can be tested independently. – Alnitak Jan 14 '16 at 15:37
  • I'll remember it. Thanks for the advices – Tân Jan 14 '16 at 15:44
  • There is no need to convert the parts to Number, strings work fine. The validation part isn't robust, it just checks format, not whether the date is actually valid or not. – RobG Jan 14 '16 at 21:23
1

Your getCurrentDateTime function was broken. I have no idea why did you overcomplicate it so much - it's just a substitute for Date, isn't it?

var getCurrentDateTime = function () {
  return new Date();
};

var parseTimeString = function (d) {  
  var dateRegex = /^([0-9]{1,2})\/([0-9]{1,2})\/([0-9]{4})\s+([0-9]{1,2}):([0-9]{1,2}):([0-9]{1,2})$/;
  var matches = dateRegex.exec(d);
  console.log(matches);
  if(!matches || matches.length<7)
    throw new Error("Invalid date.");
  var givenDate = new Date(1*matches[3],
                           1*matches[2]-1, 
                           1*matches[1],
                           1*matches[4], 
                           1*matches[5], 
                           1*matches[6]);
  var currentDate = getCurrentDateTime();
  var difference = Math.abs((currentDate.getTime() - givenDate.getTime())/1000);    

  if (difference <= 29) {
    return 'Just now'
  }
  if (difference > 29 && difference < 60) {
    return '30 seconds ago'
  }
  if (difference >= 60 && difference < 120) {
    return '1 minute ago'
  }
  if (difference >= 120 && difference < 3600) {
    return (currentDate.getMinutes() - x.getMinutes()) + ' minutes ago'
  }
  if (difference >= 3600 && difference < 7200) {
    return '1 hour ago'
  }
  if (difference >= 7200 && difference < 86400) {
    return (currentDate.getHours() - givenDate.getHours()) + ' hours ago'
  }
  if (difference >= 86400 && difference < 172800) {
    var m = givenDate.getMinutes();
    return 'Yesterday ' + givenDate.getHours() + ':' + (m < 10 ? '0' + m : m)
  }
  if (difference >= 172800) {
    var dd = givenDate.getDate(),
      MM = givenDate.getMonth() + 1,
      yyyy = givenDate.getFullYear(),
      m = givenDate.getMinutes();
    dd = dd < 10 ? '0' + dd : dd;
    MM = MM < 10 ? '0' + MM : MM;
    return dd + '/' + MM + '/' + yyyy + ' at ' + givenDate.getHours() + ':' + (m < 10 ? '0' + m : m)
  }
};



$('button').click(function () {
  var starttime = new Date();
  var asString = "14/01/2016 "+starttime.getHours()+":"+starttime.getMinutes()+":"+starttime.getSeconds();
  setInterval(showDate, 1000, asString);
  function showDate(startDate) {
    var x = parseTimeString(startDate);
    $('body').html($('<p>').text(x))
  }
  showDate(asString);
})
<script src="https://ajax.googleapis.com/ajax/libs/jquery/2.1.1/jquery.min.js"></script>
<button>Click me</button>
Tomáš Zato
  • 50,171
  • 52
  • 268
  • 778
  • When I use `+` character, I mean: try to convert `d_d.split('/')[2]` to number. And as you can see, it's wrong without any problem – Tân Jan 14 '16 at 15:16
  • @Alnitak You said: I found the problem here, here and here. So why? I still don't understand why, really. – Tân Jan 14 '16 at 15:19
  • @HappyCoding I have further edited to show you possible improvements. Regular expression allows to catch some typos in date and still produce good results. – Tomáš Zato Jan 14 '16 at 15:23
  • Can you say something about `1*`? – Tân Jan 14 '16 at 15:34
  • 1
    @HappyCoding it's to do the same job as your `+` - to convert the strings into numbers. Another way is per my answer - just pass the entire array through `.map(Number)` – Alnitak Jan 14 '16 at 15:35
  • @Alnitak yeah, you could go on javascript on that and map array on `Number` and then [use `bind` to *apply* the array on `Date` constructor](http://stackoverflow.com/a/8843181/607407). – Tomáš Zato Jan 14 '16 at 15:39
  • @TomášZato but only if the args are in the right order and with the month field decremented first! ;-) – Alnitak Jan 14 '16 at 15:40
  • @Alnitak oh yeah... well since there are no named regex groups I won't even try :D – Tomáš Zato Jan 14 '16 at 15:50
  • 3
    @TomášZato here's a one-liner for you: `var date = "14/1/2015 13:30:10"; return new Date(Date.UTC.apply(null, date.match(/^([0-9]{1,2})\/([0-9]{1,2})\/([0-9]{4})\s+([0-9]{1,2}):([0-9]{1,2}):([0-9]{1,2})$/).slice(1).map(Number).map((n, i, a) => a[i < 3 ? 2 - i : i] - ((i == 1) ? 1 : 0))))` ;) – Alnitak Jan 14 '16 at 15:58
  • @Alnitak wow nice - though one liner only if there's no lone wrap :P. But you can invoke general constructors with bind, that's why I shared the link. At least I thought it works even on built-ins. – Tomáš Zato Jan 14 '16 at 16:05
  • Ah, yes. I can't make it work with the above array, though :( Right - it needs the additional `context` variable on the front of the array. – Alnitak Jan 14 '16 at 16:08
  • 1
    shorter regexp: `^(\d\d?)\/(\d\d?)\/(\d{1,4})\s+(\d\d?):(\d\d?):(\d\d?)$` – Alnitak Jan 14 '16 at 16:21
  • shorter: `^(\d\d?/){2}(\d{4})\s(\d\d?:){2}(\d\d?)$` :-) – Tân Jan 14 '16 at 16:26
  • `new (Function.prototype.bind.apply(Date, date.match(/^(\d\d?)\/(\d\d?)\/(\d{1,4})\s+(\d\d?):(\d\d?):(\d\d?)$/).map((_,i,a)=>a[i<4?4-i:i]-+(i===2))))` – Alnitak Jan 14 '16 at 16:32
  • Sorry, missing 1 \. `^(\d\d?\/){2}(\d{4})\s(\d\d?:){2}(\d\d?)$` – Tân Jan 14 '16 at 16:40
  • That doesn't work either - it doesn't create enough capturing groups, nor discard the separates between the digit groups. – Alnitak Jan 14 '16 at 16:58
  • Your parser is far more complicated than it needs to be. Validating the date string is also very simple without a regular expression and you can't simply subtract hours and minutes to get the difference. But otherwise, good. ;-) – RobG Jan 14 '16 at 21:19
1

There are a few issues with your code. As mentioned in other answers, the getCurrentDate function isn't worth having, you may as well just do:

new Date();

When parsing a date, you need to validate more than just the pattern, you also need to validate the values (e.g. a time of 25:06:63 isn't valid). You can bundle parsing and validation in one function so that if the values aren't valid, you return a Date object with NaN as the time value (which is what ECMA-262 says to do).

Also, when doing the "time ago" part, you don't need the >= part of the comparison since each if block returns (like a case block). Putting that together:

/* Parse string in d/m/y h:m:s format to date
** If date string is invalid date, return Date with time value
** of NaN (per ECMA-262)
**
** @param {string} s - date string in format dd/mm/yyyy hh:mm:ss
** @returns {Date}
*/
function parseDMYHMS(s) {
  var b = s.split(/\D/);
  var d = new Date(b[2], --b[1], b[0], b[3], b[4], b[5]);
      
  // Validate the date string components based on the created Date
  return d && d.getMonth() == b[1] && d.getHours() == b[3] && d.getMinutes() == b[4]? d : new Date(NaN);
}

/* Return how long ago d was
**
** @param {Date} d
** @returns {string} or undefined if invalid input
*/
function timeAgo(d) {
  if (!d || !d.getTime()) return;  // Deal with falsey input, assume Date otherwise
  function pad(n){return ('0'+n).slice(-2)}
  var z = (new Date() - d) / 1e3;  // Time difference in seconds
  if (z < 30) return 'Just now';
  if (z < 60) return '30 seconds ago';
  if (z < 120) return '1 minute ago';
  if (z < 3600) return (z/60 | 0) + ' minutes ago';
  if (z < 7200) return '1 hour ago';
  if (z < 86400) return (z/3600 | 0) + ' hours ago';
  if (z < 172800) return 'Yesterday ' + d.getHours() + ':' + pad(d.getMinutes());
  return pad(d.getDate()) + '/' + pad(d.getMonth()+1) + '/' + d.getFullYear();
}

function showTimeago(s) {
  document.getElementById('div0').innerHTML = timeAgo(parseDMYHMS(s));
}  
<label for="in0">Date (d/m/y h:m:s)<input id="in0" onblur="showTimeago(this.value)" value="14/01/2016 10:03:01"></label>
<br>
<div id="div0"></div>
RobG
  • 142,382
  • 31
  • 172
  • 209