Redundant call to Drupal.checkPlain():
The source of the tag name argument in tag_exists() is from .text() method and it is compared with the result of .text() method of the selected tags so the call to checkPlain will cause comparison to fail.
For example if the tag name was "R&D", checkPlain will return "R&D" and the comparison will fail.

Case sensitivity:
The comparison doesn't normalize case but I'm not sure if case sensitive terms are something categorically unwanted.

Redundant iteration in each():
The iteration is not stopped once a match is found. each() iteration can be broken by returning false.

Here is a modified version of tag_exists() that addresses the above issues:

      function tag_exists(tag) {
        var tag = $.trim(tag.toLowerCase());
        var found = false;
        $(wrapper_sel+' '+tag_sel).each(function() {
          if($(this).text().toLowerCase() == tag) {
            found = true;
            return false; // to break out of each() iteration
          }
        });
        return found;
      }

Comments

geneticdrift’s picture

geneticdrift commented
Version: 6.x-2.1 » 6.x-2.4

Reported for the wrong version. It's actually from 6.x-2.4.

eugenmayer’s picture

eugenmayer commented

I liked the idea of being not case sensitive, added. I alos added a trim to the conditition, so we actually trim both.

 function tag_exists(tag) {
        var tag = $.trim(tag.toLowerCase());
        var found = false;
        $(wrapper_sel+' '+tag_sel).each(function() {
          if($(this).text().toLowerCase().trim() == tag) {
            found = true;
            return;
          }
        });
        return found;
      }

Applied, will be part of 2.5

Thank you fro the contribution

eugenmayer’s picture

eugenmayer commented
Status: Active » Fixed
geneticdrift’s picture

geneticdrift commented

Looks like you didn't include the "return false;" in the each() callback function. It can save unnecessary iterations once a match was found.

We can break the $.each() loop at a particular iteration by making the callback function return false. Returning non-false is the same as a continue statement in a for loop; it will skip immediately to the next iteration.

eugenmayer’s picture

eugenmayer commented
Status: Fixed » Patch (to be ported)

good catch, will update this

geneticdrift’s picture

geneticdrift commented

I'm not an expert on this subject, but I just found out that the trim() method on strings in js is relatively new (new in Firefox 3.5), so it's probably better to use jQuery's $.trim() instead.

So instead of:
if($(this).text().toLowerCase().trim() == tag)

this will probably be more compatible:
if($.trim($(this).text().toLowerCase()) == tag)

Thank you for sharing and maintaining this very useful module.

eugenmayer’s picture

eugenmayer commented

Thanks for reporting and contributing guys. Using $.trim as used above, will be included in 2.5

eugenmayer’s picture

eugenmayer commented
Status: Patch (to be ported) » Fixed

woops, status

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.