JavaScript: Refactoring looping regex

Question

How might this be improved, as it pertains to the loop and the regex replace?

var properties = { ... };
var template = element.innerHTML;

for (var name in properties) {
    template = template.replace
        (new RegExp('\\${' + name + '}', 'gm'), properties[name]);
}

element.innerHTML = template;

Is there a way I could get all the matches for /\$\{\w+\}/gm and just use those to build a new string once for the entire operation?

Community · Accepted Answer · 2017-05-23 10:33:01Z

3

I agree with Jason and Hans WRT not bothering with this from a performance perspective.

But, i would have written it differently in the first place:

element.innerHTML
  = template.replace(/[$][{](\w+)[}]/g, function(x,y){return properties[y]||x;})

Some things to keep in mind

If at all possible, you want to avoid looping over the creation of a RegExp for each iteration. Compiling them is generally considered costly. Or even generalize that to any object creation. Though not at the cost of readability/maintainability.
If you're creating RegExp dynamically, be sure the result is a RegExp, otherwise see #1 as you'll likely be able to apply it.

edited May 23, 2017 at 10:33

CommunityBot

11 silver badge

answered Feb 12, 2010 at 3:35

nicerobot

9,2556 gold badges45 silver badges44 bronze badges

Sign up to request clarification or add additional context in comments.

8 Comments

JamesBrownIsDead Over a year ago

I don't understand the function(x,y) part. The second parameter for RegEx() is the flags for the pattern, and the second parameter for replace() is the new string that returns.

Jimmy Over a year ago

the second parameter for replace is optionally a transformation function

Jimmy Over a year ago

you might want to return properties[y] || y; if you want to keep any symbols that aren't recognized

Shog9 Over a year ago

@JamesBrown: the RegExp was broken. The idea is sound though. @Jimmy: good idea...

Alan Moore Over a year ago

@Jimmy: ...or maybe return properties[y] || x; to restore the full ${foo} sequence if there's no foo property. It effectively ignores invalid tags, which is what the OP's code does (and Hans's code does, too).

|

HBP · Accepted Answer · 2010-02-12 03:12:14Z

1

I'll bite ;-)

var properties = { ... };
var template = element.innerHTML;
element.innerHTML = template.replace (
    RegExp ('\\$\\{(' + getTags (properties).join ('|') +')\\}'),
    function (m0, tag) {return properties[tag];});

function getTags (obj) {
  var tags = [];
  for (var t in obj)
    hasOwnProperty (t) && tags.push (t);
  return tags;
}

Still loops through the tags of properties (in call on getTags) but creates only one regexp object and scans the template only once.

Note that the tags names in properties should not contain special regexp characters (like . or (etc.).

I'd agree with Jason though, probably not worth the effort unless there are lots of tags or the template is very large.

answered Feb 12, 2010 at 3:12

HBP

16.1k6 gold badges30 silver badges34 bronze badges

Comments

Jason Kester · Accepted Answer · 2010-02-12 02:53:30Z

0

Inefficient as it seems, I don't think you're going to do much better than that. So long as you're not replacing more than a few dozen tokens, I'd be surprised if this was actually a bottleneck.

If your profiler hasn't identified this as being a bottleneck, I definitely wouldn't spend time rewriting it. If nothing else, it's a lot more readable than your other idea, and in the end it's probably just as fast.

answered Feb 12, 2010 at 2:53

Jason Kester

6,0519 gold badges38 silver badges40 bronze badges

Collectives™ on Stack Overflow

JavaScript: Refactoring looping regex

3 Answers 3

8 Comments

Comments

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

3 Answers 3

8 Comments

Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Related