1
\$\begingroup\$

Can someone help me make a more elegant solution to this function?

function convertToHHMM(info) {
var suffix='AM';
var hrs = parseInt(Number(info));  
var min = Math.round((Number(info)-hrs) * 60);
if (hrs >12) { hrs = hrs - 12; suffix='PM'; }
if (min < 10) {min='0'+min;}
return hrs + ':' + min + ' ' + suffix;
}
\$\endgroup\$
1
  • 2
    \$\begingroup\$ To make life easier for reviewers, please add sufficient context to your question. The more you tell us about what your code does and what the purpose of doing that is, the easier it will be for reviewers to help you. See also this meta question \$\endgroup\$
    – Mast
    Commented Apr 28, 2015 at 17:38

3 Answers 3

3
\$\begingroup\$

It's kinda hard to make it "more elegant", but it can certainly be made more correct.

Since you use Math.round, you can get a time like "3:60 PM" if you pass it 15.999. And I'm preeeetty sure there's no such thing.

There's also another bug. If you pass a value like 0.5, you should get "12:30 AM". But you don't. You get "0:30 AM".

And some nitpicking: First off, info is not a very descriptive name. It looks like you're trying to format a number of hours, with minutes as the decimals, so hours would be a better name. (Edit: or better yet fractionalHours as rolfl suggests, and which I forgot to credit.)

Also, instead of using parseInt (which is meant to turn a string into an integer), use Math.floor, which is what you actually mean: The number without any decimals. For the minutes, you could use the modulo operator (%) to get the remainder of dividing by 1 (i.e. the decimals) instead of subtracting. But either one works.

By the way, even if your input is a string like "12.3231" JavaScript's type coercion will treat it as a number, so you don't really need Number() anywhere, but it's fine to add it.

You can also do something slightly tricky by using modulo 12 on the hours, and default to 12 if the result it zero. That fixes the 0/12 bug mentioned above.

Also, fewer lines do not make things better/faster/more elegant. In this case, it just hurts readability. So use some linebreaks instead of having a line like if (hrs >12) { hrs = hrs - 12; suffix='PM'; }.

Lastly, use some indentation. It may just be a copy/paste thing when you created the question, but still.

I might do something like this, first converting everything to minutes and rounding that, then breaking it into hours and minutes afterward:

function convertToHHMM(fractionalHours) {
  var totalMinutes = Math.round(fractionalHours * 60) % (24 * 60),
      hours = Math.floor(totalMinutes / 60) % 12 || 12,
      minutes = totalMinutes % 60,
      suffix = totalMinutes >= 12 * 60 ? 'PM' : 'AM';

  if(minutes < 10) minutes = '0' + minutes;

  return hours + ':' + minutes + ' ' + suffix;
}
  • totalMinutes rolls over after rounding, so passing in 23.999 is equivalent to passing in 0, and 27 becomes 3, etc.
  • hours also roll over but start at 12 instead of zero, then 1, 2, 3 and so on.
  • minutes should be straight-forward, and
  • suffix basically looks at whether the totalMinutes are greater than or equal to noon-in-minutes.

Snippet with some test cases below (format borrowed from rolfl).

function original(info) {
var suffix='AM';
var hrs = parseInt(Number(info));  
var min = Math.round((Number(info)-hrs) * 60);
if (hrs >12) { hrs = hrs - 12; suffix='PM'; }
if (min < 10) {min='0'+min;}
return hrs + ':' + min + ' ' + suffix;
}


function reviewed(fractionalHours) {
  var totalMinutes = Math.round(fractionalHours * 60) % (24 * 60),
      hours = Math.floor(totalMinutes / 60) % 12 || 12,
      minutes = totalMinutes % 60,
      suffix = totalMinutes >= 12 * 60 ? 'PM' : 'AM';

  if(minutes < 10) minutes = '0' + minutes;

  return hours + ':' + minutes + ' ' + suffix;
}

var testCases = [
  ["12:00 AM", 0],          // midnight
  ["12:30 AM", 0.5],
  ["1:00 AM",  0.99999999],
  ["11:30 AM", 11.5],
  ["12:00 PM", 11.9999999], // noon
  ["12:00 PM", 12],
  ["11:30 PM", 23.5],
  ["12:00 AM", 23.9999999],
  ["12:00 AM", 24],
  ["1:00 AM",  25]
];

var data = "<table>";
data += "<tr><th>Input</th><th>Expected</th><th>Review func</th><th>Original func</th></tr>";
for(var i = 0, l = testCases.length ; i < l ; i++) {
  data += "<tr>";
  data += "<td>" + testCases[i][1] + "</td>";
  data += "<td>" + testCases[i][0] + "</td>";
  data += "<td>" + reviewed(testCases[i][1]) + "</td>";
  data += "<td>" + original(testCases[i][1]) + "</td>";
  data += "</tr>";
}
data += "</table>";

document.body.innerHTML = data;
body { font-family: monospace }
table { border-collapse: collapse }
td, th {
  padding: 0.2em 0.5em;
  border: 1px solid #ccc
}

\$\endgroup\$
1
\$\begingroup\$

This question really sucks, and it's a good example of why Code Reviews are important, and there is a lot to learn in it. I was very close to closing the code as "Does not work".

3 hours ago you were asked to give some clarification and context. You did not. So, let me show you how a programmer should present their code for review, because in there is a lesson too. This is approximately what your question should have looked like:

My function takes a time and formats the time in Hours and Minutes with a AM/PM suffix. The input time is in the format of a floating-point number, so, for example, 14.5 is 14 and a half hours after midnight, or 2:30PM

function convertToHHMM(info) {
    var suffix='AM';
    var hrs = parseInt(Number(info));  
    var min = Math.round((Number(info)-hrs) * 60);
    if (hrs >12) { hrs = hrs - 12; suffix='PM'; }
    if (min < 10) {min='0'+min;}
    return hrs + ':' + min + ' ' + suffix;
}

Why is that text introduction necessary? Well, for a start, your code does not tell us what info is. It could be a time-since-epoch, it could be seconds-since-start-of-day, it could be anything.

I had to struggle through WTF moments to understand why parseInt(Number(info)) was necessary. How was I supposed to know that info was already a Number, and not a String, or some other object, like a Date. I had to understand that the ParseInt was important not because the input was a String, but because it was a floating-point number and you wanted just the integral part.

OK, so, having done the hard work of understanding how your function works, and what the inputs and outputs are, this is how I would write it....

... but first, the bugs in your code.

  1. what if someone gives you the input 5.9999. Your code will output 5:60AM which makes no sense. The Math.round() of the minutes is a problem. You want to get Math.floor().
  2. your handling around noon is very problematic. You use hrs > 12 to test for noon, but, that should be hrs >= 12. As it stands, your code says the input 12.50 is 12:30AM when it should be 00:30PM.

This is approximately how I would have written your function, given the above... I have compared it with yours using some input values to test with.

function convertToHHMMX(fractionalHours) {
  var fHours = Number(fractionalHours);
  var hours = Math.floor(fHours);
  var minutes = Math.floor((fHours - hours) * 60.0);
  var fill = minutes < 10 ? '0' : '';
  var suffix = "AM";
  if (hours >= 12) {
    suffix = "PM";
    hours -= 12;
  }
  if (hours == 0) {
    // make 00:30AM / PM become 12:30AM / PM
    hours += 12;
  }
  return hours + ':' + fill + minutes + suffix;
}

function convertToHHMM(info) {
var suffix='AM';
var hrs = parseInt(Number(info));  
var min = Math.round((Number(info)-hrs) * 60);
if (hrs >12) { hrs = hrs - 12; suffix='PM'; }
if (min < 10) {min='0'+min;}
return hrs + ':' + min + ' ' + suffix;
}

var data = "YourFn     Monkey\n";
for (var t = 0; t < 24; t += 0.49999) {
  data += convertToHHMM(t) + "     " + convertToHHMMX(t) + "\n";
}
document.getElementById("output").innerHTML = data;
<pre id="output">Output</pre>

\$\endgroup\$
7
  • \$\begingroup\$ I'm confused: 12.5 should be "12:30 PM". 0.5 should be "12:30 AM". There's no zero-hour if you use AM/PM notation. Math.floor(fractionalHours) % 12 || 12 should get you the right result (albeit in a kinda tricky way) \$\endgroup\$
    – Flambino
    Commented Apr 28, 2015 at 21:30
  • \$\begingroup\$ Good point Flambino \$\endgroup\$
    – isuelt
    Commented Apr 28, 2015 at 21:33
  • \$\begingroup\$ > "3 hours ago you were asked to give some clarification and context. You did not." I'm sorry. \$\endgroup\$
    – isuelt
    Commented Apr 28, 2015 at 21:34
  • \$\begingroup\$ changing ROFL's code to this seems to work better: Instead of: if (hours >= 12) { make it if (hours > 12) { and add if (hours == 0) { hours = 12 } // midnight before the return \$\endgroup\$
    – isuelt
    Commented Apr 28, 2015 at 21:41
  • 1
    \$\begingroup\$ I remember this question. I felt the same way. I spent some long minutes trying to make sense of it. Finally I concluded that it just really sucked too much, and walked away angry, partly at the op, partly at myself to even bother about such underspecified non-programmer-ly question \$\endgroup\$
    – janos
    Commented May 8, 2015 at 5:58
1
\$\begingroup\$

It looks like your code is essentially converting military time in number format to 12-hour time.

Suggestions

  • Instead of parseInt() you can use bitwise right-shifting to efficiently strip decimals off a number. The advantages are both efficiency and avoiding potential problems when the input has a preceding 0. (EDIT: Flambino pointed out in the comments that you can also use the bitwise OR operator x | 0; to accomplish the same thing!)

    • use regular ol' bitwise rightshift x >> 0 for potentially negative values for x
    • use unsigned bitwise rightshift x >>> 0 when x is guaranteed to be positive
  • Use the modulus operator % to get the remainder when a number is divided by another number.

    • You can use this to convert the hours to the range of 12, no matter how high it goes: hrs %= 12; Note that this will convert 12 to 0, so you'd need to check for 0 and change it to 12.
    • You can use this to get the decimal remainder for calculating the minutes: var min = (+info % 1)*60 >>> 0;
    • And finally, you can use this to determine whether the converted hour will be in the morning or the afternoon: var suffix = hrs % 24 >= 12 ? "PM" : "AM";

Here's an example using those suggestions:

function convertToHHMM(info) {
    var hrs = (+(info) >>> 0);
    var min = (+(info) % 1) * 60 >>> 0;
    var suffix = hrs % 24 >= 12 ? "PM" : "AM";
    hrs %= 12;
    if (hrs === 0) {
        hrs = 12;
    }
    return hrs + ":" + (min < 10 ? "0" : "") + min + " " + suffix;
}

Side Notes

+info is functionally equivalent to Number(info) but takes up less space; that's a personal preference rather than a recommendation. If you prefer Number() then stick with it.

Similarly, I used the ternary operator (condition ? ifTrue : ifFalse) syntax a few times to keep the code concise, but you may prefer If/Else statements for readability.

Working Example

Here's a code snippet to see it in action:

document.getElementById("submit").onclick = function() {
  var input = document.getElementById("input").value;
  document.getElementById("output").innerHTML = convertToHHMM(+input);
};

function convertToHHMM(info) {
  var hrs = (+(info) >>> 0);
  var min = (+(info) % 1) * 60 >>> 0;
  var suffix = hrs % 24 >= 12 ? "PM" : "AM";
  hrs %= 12;
  if (hrs == 0) {
    hrs = 12;
  }
  return hrs + ":" + (min < 10 ? "0" : "") + min + " " + suffix;
}
<input type="text" id="input" value="1.5" />
<input id="submit" type="button" value="submit" />
<div id="output" />

\$\endgroup\$
5
  • \$\begingroup\$ Thriggle I like this code better than ROFL's. It's not as readable, but seems more elegant. Thanks for your work and informative comments. \$\endgroup\$
    – isuelt
    Commented Apr 28, 2015 at 21:55
  • 1
    \$\begingroup\$ You can also just use bitwise OR: x | 0. Shorter than shifting. \$\endgroup\$
    – Flambino
    Commented Apr 28, 2015 at 21:56
  • \$\begingroup\$ Neat! Thanks for pointing that out, Flambino \$\endgroup\$
    – Thriggle
    Commented Apr 28, 2015 at 22:09
  • 1
    \$\begingroup\$ No prob. You can also left-shift zero places, or use XOR zero (x ^ 0). Basically, any bitwise operation in JS will treat the operands as a 32-bit signed integers - i.e. drop the decimals. This despite numbers in JS being 64-bit floats in every other context. So it's just a JS oddity, but useful one. Just make sure that whoever's reading your code knows the trick too, or it'll look like voodoo :) \$\endgroup\$
    – Flambino
    Commented Apr 28, 2015 at 22:25
  • \$\begingroup\$ There's a slight advantage to using unsigned bitwise rightshift >>> over other bitwise operations if you know that the number will be positive. Very large numbers will ruin the coercion such that 3000000000.5 | 0 results in -1294967296. Unsigned bitwise rightshift will still break when it's applied to large enough numbers, but the working range is higher. \$\endgroup\$
    – Thriggle
    Commented Apr 29, 2015 at 18:24

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.