0
\$\begingroup\$

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
}
\$\endgroup\$
3
  • \$\begingroup\$ I'm not sure what you mean with the if 24 > 30 part. \$\endgroup\$
    – Mast
    Jun 29, 2015 at 16:02
  • \$\begingroup\$ 24 is the minute of the hour (1:24) - hour[i] = minute[i] > 30 ? hour[i] + 1 : hour[i] \$\endgroup\$
    – Fiat Pax
    Jun 29, 2015 at 16:04
  • \$\begingroup\$ 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. \$\endgroup\$
    – phil
    Jun 29, 2015 at 16:19

3 Answers 3

1
\$\begingroup\$
  • 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.

\$\endgroup\$
2
\$\begingroup\$

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";
}
\$\endgroup\$
2
\$\begingroup\$

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";
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.