11
\$\begingroup\$

At the moment I have this:

function getValue(str)
{
	let result = 0;
	
	var regex = /(\d+[a-z]+)/g;
	match = regex.exec(str);

	while (match != null) 
	{
		var match_str 	= match[0];
		var last_char 	= match_str[match_str.length-1];

		if ( last_char == 'h' )
			result += parseInt(match_str) * 3600;
		if ( last_char == 'm' )
			result += parseInt(match_str) * 60;
		if ( last_char == 's' )
			result += parseInt(match_str);

		match = regex.exec(str);
	}

	return result;
}

console.log( getValue("4h12m32s") );

It feels quite clumsy though. Also checking invalid values like 4hs feels difficult.

Is there any clever trick for something similar?

\$\endgroup\$
  • 2
    \$\begingroup\$ +"4h12m32s".replace(/(\d+)h(\d+)m(\d+)s/, (_, h, m, s) => (h * 3600) + (m * 60) + (s * 1)) ;) \$\endgroup\$ – user11536834 Jul 25 at 22:42
  • \$\begingroup\$ (might as well enjoy that implicit type coercion) \$\endgroup\$ – user11536834 Jul 25 at 22:49
  • 2
    \$\begingroup\$ @user11536834 I would expect that each unit is optional. The current code allows that; your solution doesn't. \$\endgroup\$ – 200_success Jul 25 at 23:17
  • \$\begingroup\$ It's probably not a good idea to mix var and const/let - you could consider replacing them all with const (or let, when the variable needs to be reassigned) \$\endgroup\$ – CertainPerformance Jul 27 at 9:53
14
\$\begingroup\$
  • getValue(str) is such a vague name for the function and its parameter, it could mean anything! Furthermore, "get" implies that this is a getter function that retrieves something, which is not the case.
  • Your regex is ineffective. Capturing parentheses could be useful, but you didn't actually use them right, such that you ended up having to pass a dirty string to parseInt() and extract the last character the harder way.
  • You neglected to scope match, such that it acts as a global variable. The regex-matching statement is written twice; the assignment could be done within the loop condition instead.
  • The if statements should be an if-else chain, since the conditions are mutually exclusive. However, since the branches are all so similar, a lookup table would be more elegant.

function durationSeconds(timeExpr)
{
	var units = {'h': 3600, 'm': 60, 's': 1};
	var regex = /(\d+)([hms])/g;

	let seconds = 0;
	var match;
	while ((match = regex.exec(timeExpr))) 
	{
		seconds += parseInt(match[1]) * units[match[2]];
	}

	return seconds;
}

console.log( durationSeconds("4h12m32s") );

Alternatively, if you expect that the units will be in the conventional order, you don't have to loop at all.

function durationSeconds(timeExpr)
{
	var match = /^(?:(\d+)h)?(?:(\d+)m)?(?:(\d+)s)?$/.exec(timeExpr);
	return 3600 * (parseInt(match[1]) || 0)
	       + 60 * (parseInt(match[2]) || 0)
	       +      (parseInt(match[3]) || 0);
}

console.log( durationSeconds("4h32s") );

\$\endgroup\$
  • \$\begingroup\$ Some of your mentions were because I have extracted this from my current code base (and messed up a bit with copy pasting), but this is exactly the knowledge and cleverness I was looking for. Despite that the second one is more consise I think your first example is great and super readable. Thanks for your effort. \$\endgroup\$ – Dirk Boer Jul 26 at 9:45
  • \$\begingroup\$ Btw - one last question: why the double parentheses within the while loop in your first example? \$\endgroup\$ – Dirk Boer Jul 26 at 9:48
  • 3
    \$\begingroup\$ One common mistake is to write while (a = 1) when you meant while (a == 1). The extra parentheses is a convention to emphatically say "yes, that really is an assignment". \$\endgroup\$ – 200_success Jul 26 at 9:56
  • \$\begingroup\$ Does eslint have that warning, and does the double parenthesis silence it? \$\endgroup\$ – Barmar Jul 27 at 17:01
10
\$\begingroup\$

Named Capture Groups

JavaScript RegExp has named capture groups that can make life a lot simpler when dealing with complicated RegExp. Combined with destructuring assignment you can extract the named hours minutes and seconds as follows.

function toSeconds(time) {
    const {groups: {h = 0, m = 0, s = 0}} = /(?<h>\d*)h(?<m>\d*)m(?<s>\d*)/i.exec(time);
    return h * 3.6e3 + m * 60 + s * 1;  // * 1 to coerce s to Number
}

Missing digits are set to zero in the assignment defaults.

However this is limited to strings that have hours, minutes, and seconds in the correct order (hence no need to match the "s") and will throw an error if there is a problem.

A more robust solution

You can also reduce the array created by symbol.matchAll (it returns an iterator that you convert to an array via spread operator)

It RegExp[symbol.matchAll] is the same call as String.matchAll(RegExp)

To handle as many variations as possible you can convert the time string to lowercase, soak up white spaces, allow for fractions, multiple periods, and negative periods.

Using an IIF to wrap the periods constant via closure the function looks like

const toSeconds = (() => {
    const periods = {h: 3600, m: 60, s: 1};
    return time => [.../(\-*\d*\.*\d*)\W*([hms])/g[Symbol.matchAll](time.toLowerCase())]
        .reduce((time, [, digits, type]) => periods[type] * digits + time, 0);
})();

Or via the string

const toSeconds = (() => {
    const periods = {h: 3600, m: 60, s: 1};
    return time => [...time.toLowerCase().matchAll(/(\-*\d*\.*\d*)\W*([hms])/g)]
        .reduce((time, [, digits, type]) => periods[type] * digits + time, 0);
})();

To combat the readability the next version creates some extra variables to segregate the logic parts a little

const toSeconds = (() => {
    const periods = {h: 3600, m: 60, s: 1};
    const extractHMS = /(\-*\d*\.*\d*)\W*([hms])/g;
    const sumSeconds = (time, [, digits, type]) => periods[type] * digits + time;

    return time => [...time.toLowerCase().matchAll(extractHMS)].reduce(sumSeconds, 0);
})();

The snippet below shows some of the results of a variety of inputs.

const toSeconds = (() => {
    const periods = {h: 3600, m: 60, s: 1};
    return time => [.../(\-*\d*\.*\d*)\W*([hms])/g[Symbol.matchAll](time.toLowerCase())]
        .reduce((time, [, digits, type]) =>periods[type] * digits + time, 0);
})();



"1h,1m,1s,1,,1s2m3h,3h2m1s,2H2M2S,1h 1H1s1 S1m1M,1.1s,1.2s,s,1h-5m,1 1s,hms"
    .split(",")
    .forEach(time => log("\"" + time + "\" =" , toSeconds(time)+" seconds"));
    
function log(...data) {
    document.body.appendChild(
        Object.assign(
            document.createElement("div"), {textContent: data.join(" ")}
        )
    )
}

BTW in Javascript we put...

  • the opening { on the same line as the statement,
  • use camelCase for naming.

And from many years of C style language experience I would advise you to always delimit statement blocks with { } eg Bad if (foo) bar = foo, Good if (foo) { bar = foo }

\$\endgroup\$
  • 1
    \$\begingroup\$ This code might be more robust, but it's also A LOT harder to read IMHO. \$\endgroup\$ – Nzall Jul 26 at 9:08
  • 2
    \$\begingroup\$ @Nzall Modern JS is progressing to less a less verbose style that to many is less readable. I will update with a more readable version. \$\endgroup\$ – Blindman67 Jul 26 at 9:22
  • 1
    \$\begingroup\$ Very nice. Might be worth mentioning that matchAll and named captures don't have full browser support yet, but this is surely the way forward (old regex functions all have some unfortunate deficiencies, aside from replace, which is awkward to use when not actually replacing something). \$\endgroup\$ – user11536834 Jul 26 at 13:09
  • \$\begingroup\$ One other idea for the first solution could be to whack everything into Date.UTC, with 1970, 0, 1 as the first arguments, then multiply by 0.001; then you could scrap periods and do it all in one shot (a bit funky, but makes it really clear we're dealing with a timestamp). \$\endgroup\$ – user11536834 Jul 26 at 13:17
3
\$\begingroup\$

In addition:

  • "Always specify a radix when using parseInt." -- MDN parseInt documentation
  • When getValue() is given a malformed string, it ignores the malformed parts without letting anyone know something went wrong. For example, 2 h 500 m 600 s returns 0. It is better to throw an error when the input isn't valid (so the caller can more easily figure out why the values it got back are wrong).
  • If you expect requirements to change in the future, you should look into harnessing the power of a real parser (or parser generator). For example, what changes do you need to do to:
    • Add support for uppercase letters: 1H2D3M
    • Allow spaces: 30h 50 m 10s
    • Allow input to be in any order: 5s 10m
    • Allow multiple occurrences or no occurrences of a unit: 30h 10h or 10s
    • Add new units: d (days) or ms (milliseconds), etc.

Here is what that looks like using a parser generator (PEG.js):

// Duration to Seconds Grammar
// ==========================
//
// Accepts expressions like "4h53m12s" or "4H 33M 12S" 
// and computes the total number of seconds.

start
  = total

total
  = left:subtotal right:total { return left + right; }
  / subtotal

subtotal
  = left:integer right:day { return left * 86400; }
  / left:integer right:hour { return left * 3600; }
  / left:integer right:minute { return left * 60; }
  / left:integer right:second { return left; }
  / left:integer right:millisecond { return left * .001; }

day
  = whitespace [dD]

hour
  = whitespace [hH]

minute
  = whitespace [mM][^sS]

second
  = whitespace [sS]

millisecond
  = whitespace [mM][sS]

integer
  = whitespace [0-9]+ { return parseInt(text(), 10); }

whitespace
  = [ \t\n\r]*
  • Test it out in the right-hand pane
  • Choose your options and click Download Parser
\$\endgroup\$
  • \$\begingroup\$ Nice answer. That first bullet point is a really easy trap to fall into (as in OP and accepted answer here). I'd rather avoid parseInt entirely than give it a second look to make sure it's used correctly. AFAIK implicit conversion has never parsed leading zeros as octal (at least, not from ES3 forward). \$\endgroup\$ – user11536834 Jul 26 at 23:13

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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