3
\$\begingroup\$

Please check if this date filtering function is inefficient code. It seems so to me, but I can't figure out the other way around it.

function makeTime () {
    /////////////////////////////////
    //Create a filtered Time/////////
    var timesStamp = new Date();
    var time = timesStamp.toLocaleTimeString();
    var timeArray = time.split(":");
    var spaceDeliTime = timeArray.join(" ");
    var rawTime = spaceDeliTime.split(" ");
    var extrTime = rawTime.splice(2,1);
    var AMPM = rawTime.splice(2,1);
    return rawTime.join(":")+' '+AMPM;
}
\$\endgroup\$
  • \$\begingroup\$ I suggest looking into moment.js to solve this next time. \$\endgroup\$ – omgimanerd Aug 3 '17 at 21:30
2
\$\begingroup\$

You are right that slicing and dicing the string representation of a date is not the right way. Parsing a local time string without a specific locale is even worse, and asking for trouble.

A Date object in JavaScript has getHours() and getMinutes() methods. The result will be in 24-hour format. If you need, you can convert that to am-pm format based on the hour. The resulting code will be more efficient and more predictable than parsing the string representation.

For example:

var timestamp = new Date();
var hours = timestamp.getHours();
var ampm;
if (hours < 12) {
    ampm = 'AM';
} else {
    ampm = 'PM';
    if (hours > 12) {
        hours -= 12;
    }
}    

For the record, this code to get the time part in rawTime is very poor:

var timeArray = time.split(":");
var spaceDeliTime = timeArray.join(" ");
var rawTime = spaceDeliTime.split(" ");
var extrTime = rawTime.splice(2,1);

For one thing, the sequence of split by colons -> join by spaces -> split by spaces is really silly. Probably you could have achieved the same thing by simply taking a substring.

The most disturbing part is the splice at the end: the result is stored in extrTime but never used. On closer look it turns out the main purpose of that splice was its side effect of mutating the rawTime array. The logic would have been much more clear if you used array indexes to get the segments that you want rather than slicing and dicing an array.

If parsing the string representation, this would have been a simpler, more efficient, less confusing solution:

function makeTime () {
    var timesStamp = new Date();
    var time = timesStamp.toLocaleTimeString();
    var timePart = time.substr(0, time.indexOf(":", 2))
    var spaceAndAmPmPart = time.substr(time.lastIndexOf(' '))

    return timePart + spaceAndAmPmPart;
}

But I still recommend the different approach using getHours and getMinutes.

\$\endgroup\$
  • \$\begingroup\$ Can you provide the way to approach the getHours and getMinutes function..?? Any reference \$\endgroup\$ – ChanX Feb 14 '15 at 8:54
  • \$\begingroup\$ I added an example code for deriving the AM-PM based on the hours. It's really simple though, I'm surprised you ask for it. Taking this and .getMinutes(), you can easily concatenate to your desired format. For the record, here on Code Review we focus on reviewing code and giving advice. Don't expect complete alternative implementations in general, that's your job. \$\endgroup\$ – Stop ongoing harm to Monica Feb 14 '15 at 10:33
  • \$\begingroup\$ Thanks for the help provided.. I made my AM PM by splicing the AMPM from the toLocaleTimeString ... :) \$\endgroup\$ – ChanX Feb 15 '15 at 5:17
  • \$\begingroup\$ I advised against relying on the locale, explained in my review. Anyway, is there anything else I can help you with in this review? \$\endgroup\$ – Stop ongoing harm to Monica Feb 15 '15 at 19:32
  • \$\begingroup\$ Thank you very much for the review and i hope i will stop using locale soon but right now i have launch the app in a short time and thats the reason i am using the locale for now.. \$\endgroup\$ – ChanX Feb 17 '15 at 6:29

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.