Rewriting hours

Question

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
}

• Mast: I'm not sure what you mean with the if 24 > 30 part.
• Fiat Pax: 24 is the minute of the hour (1:24) - hour[i] = minute[i] > 30 ? hour[i] + 1 : hour[i]
• phil: 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.

Answer 1

• 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.

Answer 2

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

Answer 3

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