1
\$\begingroup\$

The following regex is meant to improve stringmatching by sanitizing the user -defined commandPrefix and checking for whitespace and line-endings after the specified text. command will generally be a single word, hence not escaped.

var prefix = commandPrefix.replace(/[-\/\\^$*+?.()|[\]{}]/g, '\\$&');
return text.test("/^" + prefix + command + "(?=\s|$)/");

Can this be improved for code readability? And am I missing any regex special characters in the sanitation?

\$\endgroup\$
  • 1
    \$\begingroup\$ You can remove - , ] and } that are not special characters. Readability is a very relative idea, what is more readable: 2 lines of straight forward code or 15 lines for the same thing ( in a "readable" way)? IMO, the only thing you can do to improve the readability is to put a comment before the two lines. (assuming it could be useful for someone) \$\endgroup\$ – Casimir et Hippolyte Feb 28 '16 at 10:35
  • 1
    \$\begingroup\$ To build a pattern by concatenation, you need to use the RegExp constructor that can take a string as parameter. You can't write something like "/^" + prefix + command +..., a RegExp instance isn't a string. As an aside Code Review is for working code. \$\endgroup\$ – Casimir et Hippolyte Feb 28 '16 at 10:42
  • 1
    \$\begingroup\$ Other thing, you can change (?=\s|$) to (?!\S) \$\endgroup\$ – Casimir et Hippolyte Feb 28 '16 at 10:44
  • \$\begingroup\$ @CasimiretHippolyte Thanks, I'll make those changes. And I was told the code worked so didn't try to check out. Would definitely do so in the future. Feel free to compile these into an answer :) \$\endgroup\$ – Paras Feb 28 '16 at 10:46
  • 1
    \$\begingroup\$ normally I use \b unless (?!\S) is exactly needed (i.e. words are delimited by whitespaces only, but not by punctuation). \$\endgroup\$ – Nishi Feb 28 '16 at 16:16
2
\$\begingroup\$

First, /pattern/ is the literal notation of a RegExp object and not a string, "/pattern/" is a string and nothing more.

If you want to concatenate several strings to build your pattern and then to obtain a RegExp object, you need to use the constructor:

var re = new RegExp('^' + prefix + command + '(?!\\S)');
return re.test(text);

(Note that when you pass a string to the RegExp constructor, you need to escape the backslashes, since to figure a literal backslash in a string you need to escape it.)

(test is a RegExp method, not a String method: RegExp.prototype.test())


About special characters: -, } and ] are not special characters and don't need to be escaped. Note that { is read as a literal character too, but only if it isn't the start of a quantifier {n}, {m,n}, {m,}. Except for these special situations, you don't need to escape it when you write a pattern by hand, but here it's easier to escape it systematically instead of testing if it is or not the start of a quantifier. (if the escape is useless, it will be ignored)

Since you will use the RegExp constructor with a string as first parameter (Since ECMAScript 6, this parameter can also be in literal notation), you no longer need to escape the delimiter / that is only used in the literal notation. You can remove it too:

var prefix = commandPrefix.replace(/[.?*+\\|{()[^$]/g, '\\$&');

About the readability, no need to make things more complicated than they are, a simple comment before the line should suffice.

Since escaping special regex characters is a basic task, and if you project to use it several times, you can build a function:

function regEscape(mystr) {
    return mystr.replace(/[.?*+\\|{()[^$]/g, '\\$&');
}

or, why not, adding it to the String methods:

String.prototype.regEscape = function() {
    return this.replace(/[.?*+\\|{()[^$]/g, '\\$&');
};

...

function ... (...) {
    ...
    var re = new RegExp('^' + commandPrefix.regEscape() + command + '(?!\\S)');
    return re.test(text);
}
\$\endgroup\$
  • 1
    \$\begingroup\$ That helped with a lot of my doubts/ideas! Thanks \$\endgroup\$ – Paras Feb 28 '16 at 17:37
2
\$\begingroup\$

If you want code readability, check the escape-string-regexp solution from @CoolAJ86


The Long Answer

If you're going to use the function above at least link to this stack overflow post in your code's documentation so that it doesn't look like crazy hard-to-test voodoo.

var escapeRegExp;

(function () {
  // Referring to the table here:
  // https://developer.mozilla.org/en/JavaScript/Reference/Global_Objects/regexp
  // these characters should be escaped
  // \ ^ $ * + ? . ( ) | { } [ ]
  // These characters only have special meaning inside of brackets
  // they do not need to be escaped, but they MAY be escaped
  // without any adverse effects (to the best of my knowledge and casual testing)
  // : ! , = 
  // my test "~!@#$%^&*(){}[]`/=?+\|-_;:'\",<.>".match(/[\#]/g)

  var specials = [
        // order matters for these
          "-"
        , "["
        , "]"
        // order doesn't matter for any of these
        , "/"
        , "{"
        , "}"
        , "("
        , ")"
        , "*"
        , "+"
        , "?"
        , "."
        , "\\"
        , "^"
        , "$"
        , "|"
      ]

      // I choose to escape every character with '\'
      // even though only some strictly require it when inside of []
    , regex = RegExp('[' + specials.join('\\') + ']', 'g')
    ;

  escapeRegExp = function (str) {
    return str.replace(regex, "\\$&");
  };

  // test escapeRegExp("/path/to/res?search=this.that")
}());
\$\endgroup\$
  • \$\begingroup\$ Thanks for the linked answer and the next best readability option. \$\endgroup\$ – Paras Feb 28 '16 at 18:09

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.