Rewriting hours

I am just trying to rewrite hours; say 1:24PM, if 24 > 30 then it is 2PM, otherwise it is 1PM.

I really doubt that I am doing it right, first I am sure it looks so amateur, second I am not sure if it is optimised.

Can you provide me a better approach to this?

hours = ["3:42am", "6:37pm", "1:24pm", "2:11am", "8:30am", "1:51am", "12:03am", "11:18pm", "12:28pm", "3:46am", "10:27pm", "9:47am", "12:07am", "8:28pm", "8:41am", "10:43pm", "11:55pm", "10:57pm", "12:43pm"]

function SplitHours(hours) {
let hourObject = {}
let hour = []
let minute = []
let hourPeriod = []
let i
for (i = 0; i < hours.length; i += 1) {
hour[i] = parseInt(hours[i].match(/^[^\:]*/gi)[0])
minute[i] = parseInt(hours[i].match(/[^:]*(?=pm|am)/gi)[0])
hourPeriod[i] = hours[i].match(/([A-Za-z])\w+/gi)[0]

if (hourPeriod[i] == "pm" && !(parseInt(hour[i]) == 12)) {
hour[i] = parseInt(hour[i]) + 12
}

hour[i] = minute[i] > 30 ? hour[i] + 1 : hour[i]

if (hour[i] == 13 && hourPeriod[i] == "am") {
hour[i] = 1;
hourPeriod[i] = "pm"
} else if (hour[i] == 13 && hourPeriod[i] == "pm") {
hour[i] = 1;
hourPeriod[i] = "am"
}
}

hourObject.hour = hour
return hourObject
}

• I'm not sure what you mean with the if 24 > 30 part.
– Mast
Jun 29, 2015 at 16:02
• 24 is the minute of the hour (1:24) - hour[i] = minute[i] > 30 ? hour[i] + 1 : hour[i] Jun 29, 2015 at 16:04
• is it possible to do all three regex matches all in one go? You're running three regex operations when you could possibly run 1? Just a thought; I don't know if it would work.
– phil
Jun 29, 2015 at 16:19

• Careful with let. Lots of runtimes still don't support the new language features.

• Indentation is wrong, but that might just be copy/paste thing.

• SplitHours should be splitHours since it's just a function, not a constructor.
Of course, it should perhaps be roundHours, since that's more descriptive. (Darn, Janos beat me to it.)

• The regexes can be combined into one and simplified: /^(\d+):(\d+)(am|pm)$/i • The g flag for the regex does nothing, since you've anchored your matching to the start or end of the string. • I'd say that you should round up from 30 minutes (i.e. use >= 30) Basically, it can all be simplified. I'm assuming your input and output formats are fixed; this isn't a full time parser (no support for 24-hour times, no support for seconds, no handling of nonsensical times, etc. etc.): function roundHour(string) { return string.replace(/^(\d+):(\d+)(am|pm)$/i, function (_, hours, minutes, meridiem) {
hours = parseInt(hours, 10);
minutes = parseInt(minutes, 10);

if(minutes >= 30) {
hours += 1;
}

if(hours > 12) {
meridiem = meridiem.toLowerCase() == "am" ? "pm" : "am";
hours -= 12;
}

return String(hours) + meridiem;
});
}


If the input doesn't match the regex, the string is just returned unaltered.

You could reduce, mainly to shorten the code

if (hour[i] == 13 && hourPeriod[i] == "am") {
hour[i] = 1;
hourPeriod[i] = "pm"
} else if (hour[i] == 13 && hourPeriod[i] == "pm") {
hour[i] = 1;
hourPeriod[i] = "am"
}


to

if (hour[i] == 13) {
hour[i] = 1;
hourPeriod[i] = hourPeriod[i] == "am"?"pm":"am";
}


As you suspected, yes, this is awful.

First of all, the function is named one thing, and it does something else. It does something more. A lot more. This shouldn't be one function. This should be decomposed to multiple smaller functions, each with a single responsibility. For example:

• parseTimeStr(timeStr) : parse a time string like 3:42am to its components, 3, 42, am
• roundToHour(timeStr) : take a time string and round it to hour
• roundAllToHour(list) : take a list of time strings and return a list of rounded time strings

As an extra to, here's an example to match all the time elements with one regex:

var match = /(\d\d?):(\d\d)(am|pm)/.exec(timeStr);
var hour = match[0];
var minute = match[1];
var am = match[2] == "am";