Date-filtering function

Question

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;
}

• omgimanerd: I suggest looking into moment.js to solve this next time.

Answer 1

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.

• ChanX: Can you provide the way to approach the getHours and getMinutes function..?? Any reference
• janos: 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.
• ChanX: Thanks for the help provided.. I made my AM PM by splicing the AMPM from the toLocaleTimeString ... :)
• janos: I advised against relying on the locale, explained in my review. Anyway, is there anything else I can help you with in this review?
• ChanX: 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..