Skip to main content
added 1 character in body
Source Link
200_success
  • 144.2k
  • 22
  • 188
  • 473
  • 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") );

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

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

Source Link
200_success
  • 144.2k
  • 22
  • 188
  • 473

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