program with recursive function

Question

I'm pretty stumped on this recursive function. I'm using it to highlight words in a text box, but it's giving me some strange output:

 should be:
 #define
 testing #define

 but instead it's:

 testing #define
 testing #define

here's the code

function replace_all( text, old_str, new_str ){

    index_of = text.indexOf( old_str );

    if( index_of != -1 ){

            old_text = text.substring( 0, index_of );
            new_text = text.substring( index_of + old_str.length );

            new_text = replace_all( new_text, old_str, new_str );

            text = old_text + new_str + new_text;
    }
    return text;
}

Any ideas on what's wrong with the function? It seems to be replacing all the old keywords with the last one found.

What's wrong with function replace_all(text,ostr,nstr) {return text.replace(new RegExp(ostr,"g"),nstr);}? — Niet the Dark Absol
– Niet the Dark Absol, Commented Apr 1, 2013 at 4:37
@Kolink that worked. I've never looked up regular expressions but it seems like that does the job. — tay10r
– tay10r, Commented Apr 1, 2013 at 4:42

jfriend00 · Accepted Answer · 2013-04-01 04:46:21Z

3

You need to at least declare all your variables in the function as local variables by adding var in front of their first use.

function replace_all( text, old_str, new_str ){

    var index_of = text.indexOf( old_str );

    if( index_of != -1 ){

            var old_text = text.substring( 0, index_of );
            var new_text = text.substring( index_of + old_str.length );

            new_text = replace_all( new_text, old_str, new_str );

            text = old_text + new_str + new_text;
    }
    return text;
}

By not using var, your variables are global and each invocation of replace_all will be sharing the same copies of variables which will mess up the recursion as recursive calls will mess up the state of the higher levels calls. If the variables are all local variables, then each function invocation has it's own set of variables and one recursive call does not mess up the others.

Plus, it is generally always a good idea to limit the scope of your variables to as local a scope as practical and avoid globals whenever possible. Implicit globals variables like you have here are particular evil because they can easily lead to accidental misuse.

As Kolink suggested, you might just want to do a single global .replace() operation using a regex search/replace. You would need to make sure that any regex special characters in the search string were properly escaped though.

edited Apr 1, 2013 at 4:46

answered Apr 1, 2013 at 4:38

jfriend00

711k104 gold badges1.1k silver badges1k bronze badges

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

1 Comment

tay10r Over a year ago

that makes sense. I thought that was happening but had no idea that was the difference between using var

Niet the Dark Absol · Accepted Answer · 2013-04-01 12:32:23Z

1

Converting my comment to an answer:

This would be a lot easier like so:

function replace_all(text,old_str,new_str) {
    var old_regex = old_str).replace(/[.\\+*?\[\^\]$(){}=!<>|:-]/g, '\\$&');
    // the above line escapes any characters that may interfere with regex behaviour
    return text.replace(new RegExp(old_regex,"g"),new_str);
}

answered Apr 1, 2013 at 12:32

Niet the Dark Absol

326k86 gold badges480 silver badges604 bronze badges

Collectives™ on Stack Overflow

program with recursive function

2 Answers 2

1 Comment

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

1 Comment

Comments

Your Answer

Sign up or log in

Post as a guest

Related