Javascript function tidy up

Question

Below you'll see a javascript/jQuery function I'm currently using:

            var forward = new RegExp('forward'),
                backward = new RegExp('backward'),              
                left = new RegExp('left'),      
                right = new RegExp('right');                

            if ( tweet.message.match(forward) ) {
                console.log('forward');
                body.addClass('forward');
                bodyRemoveClass();                          
            }

            if ( tweet.message.match(backward) ) {
                console.log('backward');
                body.addClass('backward');
                bodyRemoveClass();                          
            }

            if ( tweet.message.match(left) ) {
                console.log('left');
                body.addClass('left');
                bodyRemoveClass();                          
            }

            if ( tweet.message.match(right) ) {
                console.log('right');
                body.addClass('right');
                bodyRemoveClass();                          
            }

Everything works just fine, but I'm not 100% happy with the way it's written. Basically what that does is, check if a given keyword (forward, backward, left or right)
are in a tweet (tweet.message)

I would like to know if there is a simple/cleaner way to achieve this.

Sorry but there is no online example...

Thank you

Michael Berkowski · Accepted Answer · 2011-12-09 22:02:07Z

There's no need to use match() with Regexp here. You can do simple string matching with indexOf(). You can then avoid declaring all the Regexp at the beginning.

        if ( tweet.message.indexOf("forward") > -1) {
            console.log('forward');
            body.addClass('forward');
            bodyRemoveClass();                          
        }

        if ( tweet.message.indexOf("backward") > -1) {
            console.log('backward');
            body.addClass('backward');
            bodyRemoveClass();                          
        }

        if ( tweet.message.indexOf("left") > -1) {
            console.log('left');
            body.addClass('left');
            bodyRemoveClass();                          
        }

        if ( tweet.message.indexOf("right") > -1) {
            console.log('right');
            body.addClass('right');
            bodyRemoveClass();                          
        }

However, this is much more neatly accomplished with an array of classes:

// Store your 4 classes in an array
var classes = ["forward","backward","left","right"];
for (var i = 0; i<classes.length; i++) {

  // Check each class appearing in the message:
  if ( tweet.message.indexOf(classes[i]) > -1) {
     console.log(classes[i]);
     body.addClass(classes[i]);
     bodyRemoveClass();                          
  }
}

jfriend00 · Accepted Answer · 2011-12-09 22:12:55Z

You can use a table-drive approach like this:

var tags = ["forward", "backward", "left", "right"];
for (var i = 0; i < tags.length; i++) {
    if (tweet.message.indexOf(tags[i]) != -1) {
        body.addClass(tags[i]);
        bodyRemoveClass();
    }
}

Keep in mind that any time you're repeating the same logic and the same strings over and over again, you should find a way to either drive your logic by iterating through a table, break the code out into a common function or something that prevents you from repeating code over and over again. The popular lingo in software development these days for this is DRY (don't repeat yourself).

There was also no reason for test each individual match with a regular expression. While they can be very handy when needed, .indexOf() is a lot faster when that's all that is needed.

JayC · Accepted Answer · 2011-12-09 22:08:30Z

3

function TweetMatch(classToAdd){
    if ( tweet.message.match(new RegExp(classToAdd))) {
        console.log(classToAdd);
        body.addClass(classToAdd);
        bodyRemoveClass();                          
    }
}
TweetMatch('forward');
TweetMatch('backward');
TweetMatch('left');
TweetMatch('right');

etc. (I see fellow commenters have gone ahead and made arrays to iterate over... depends upon how many directions you want :-D)

answered Dec 9, 2011 at 22:08

JayC

7,1612 gold badges30 silver badges42 bronze badges

Comments

Wayne · Accepted Answer · 2011-12-09 22:12:55Z

1

This can be drastically shortened by capturing the matched phrase:

var match = tweet.message.match(/forward|backward|left|right/g);
for (var i = 0; match && i < match.length; i++) {
    console.log(match[i]);
    body.addClass(match[i]);
    bodyRemoveClass();
}

The only thing different in each of those if blocks is the phrase to be matched (which is the same as the name of the class to be added). Repetition like that can (and should) almost always be avoided.

edited Dec 9, 2011 at 22:12

answered Dec 9, 2011 at 22:05

Wayne

60.5k15 gold badges135 silver badges129 bronze badges

4 Comments

Aaron Over a year ago

this will match a maximum of one of the values in the regex, whereas the original function made it possible to match all of them at once.

Wayne Over a year ago

@Aaron - That's not true (did you see my edit?). Actually, it has the opposite problem. It will loop over all matches, including duplicates.

Aaron Over a year ago

Oops, I was looking at the original if statement. Maybe I should refresh once in a while, eh?

Wayne Over a year ago

@Aaron - Well, I should have noticed that in the first place; then I wouldn't have needed the edit :)

Aaron · Accepted Answer · 2011-12-09 22:05:52Z

1

var directions = new Array('forward', 'backward', 'left', 'right');
$(directions).each(function()
{
    var regX = new RegExp(this);
    if (tweet.message.match(regX))
    {
        console.log(this);
        body.addClass(this);
        bodyRemoveClass();
    }
});

answered Dec 9, 2011 at 22:05

Aaron

5,2471 gold badge21 silver badges21 bronze badges

2 Comments

jfriend00 Over a year ago

Are you trying to use jQuery here? I don't think $(directions).each() will work and the $() function doesn't accept a plain array of strings. Perhaps you meant $.each(directions, fn).

jfriend00 Over a year ago

note that there is nothing on the jquery() doc page that says you can pass an array of strings to that function. I guess I'm not saying that it doesn't work, but it sure isn't documented. It sure seems like $.each() which is documented would be a wiser choice.

Collectives™ on Stack Overflow

Javascript function tidy up

5 Answers 5

Comments

Comments

Comments

4 Comments

2 Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

5 Answers 5

Comments

Comments

Comments

4 Comments

2 Comments

Your Answer

Sign up or log in

Post as a guest

Related