19
\$\begingroup\$

I wrote the following to check whether a string ends with a second string.

function isEndOf(origin, target) {
  return (origin.substr(target.length * -1, target.length) === target);
}

Usage:

isEndOf("A long time ago in a galaxy far, far away", "far away")

This takes a substring with the size of the target string counting from the end of the string and checks whether it's equal to the target string. Quite straightforward. However, I'm concerned about my usage of target.length * -1. This smells like using the wrong method altogether.

Is there a more idiomatic approach for writing such a basic function? Would slicing be more appropriate? The naming can probably be improved.

\$\endgroup\$
  • \$\begingroup\$ Pick one: stackoverflow.com/questions/280634/endswith-in-javascript It definitely should be an extension of the String prototype. \$\endgroup\$ – Jeroen Vannevel Dec 26 '15 at 17:17
  • 2
    \$\begingroup\$ @JeroenVannevel There was a discussion about such beneath one of the answers on my last question. Can you explain why it's a good idea here? \$\endgroup\$ – Mast Dec 26 '15 at 17:22
  • \$\begingroup\$ It's the same as creating an extension method in C# -- you're extending the behaviour of a specific type with a specific functionality. The compatibility issue is moot: if a new function is introduced that does this for you then you just remove your own definition and that's that. Provide unit tests and you will know if a breaking change is introduced which you can act accordingly upon. \$\endgroup\$ – Jeroen Vannevel Dec 26 '15 at 17:45
  • 1
    \$\begingroup\$ I'd also name the method with the much more common endsWith instead. isEndOf seems particularly unfortunate with that parameter order, because to me, the code reads as "Is A long time ago in a galaxy far, far away the end of far away?" \$\endgroup\$ – millimoose Dec 27 '15 at 12:55
  • 2
    \$\begingroup\$ @JeroenVannevel not to beat a dead horse but Mast and I had an extensive discussion on this as well as in comments on Mast's previous question, but tl;dr C# is not JavaScript, extension methods are not the same as prototype extensions. In JavaScript extensions to String are global - this is not the case in C#, where you can include/exclude a specific namespace. \$\endgroup\$ – Dan Pantry Dec 28 '15 at 7:07
24
\$\begingroup\$

There's no need for the 2nd argument in substr. So you could just do:

origin.substr(target.length * -1) === target

or

origin.slice(target.length * -1) === target

Same deal.

Edit: Whoops. Actually it's not quite the same deal. As Sumurai8 commented on Simon's answer, MDN says that JScript does not handle negative offsets for substr. However, slice has no such caveat. So long story short: Use slice, not substr.

Instead of * -1, you could also use a unary minus:

origin.slice(-target.length) === target

One thing you might want, though, is to check target before you try accessing its length property. If target == undefined you'll get an error. Something simple like this should do:

if(!target) return false;

One gotcha though: It'll return false if target is an empty string, since empty strings are false'y. However, it doesn't really make much sense to check if string ends with an empty string, so perhaps that's fine? Otherwise, do a typeof check (like the one below) on target.

And you might want to check origin too. However, here you just want to check that it's a string - not if it's just false'y. You'll be calling slice on it, so it must be a string or you'll get an error, whereas the worst that can happen if target isn't a string (but is truth'y) is that target.length is undefined.

So to check origin do:

if(typeof origin !== 'string') return false;

By the way, without the target-is-false'y check, if target is a string but also empty, you'll get some tricky results:

Origin        Target         Result
==========================================
"foo"         ""             false
""            "foo"          false
""            ""             true

Results #2 makes sense, but results #1 and #3 are perhaps a little strange, because, again, what does it mean to have an string end with an empty string? The results are due to slice(-0) being the same as slice(0), i.e. returning the whole string. So in effect you're checking:

"foo".slice(0) == "" // => "foo" == "" (false)
"".slice(0) == ""    // => "" == "" (true)

Kinda confusing.

If you want an empty target string to always return true, you could do:

if(typeof origin !== 'string') return false;
if(typeof target !== 'string') return false;
if(!target) return true;

I'm aware my code has a lot of white-space for JavaScript standards. I like to think it keeps code readable. The naming can probably be improved.

Whitespace seems just fine to me! Don't know what JS you're used to, but I see absolutely no reason to change anything about whitespace usage.

I would drop the outer parentheses though, like Simon suggested.

As for naming, I'd call it endsWith, or stringEndsWith.

You could also extend the String prototype with an endsWith method, but generally it's better to leave native prototypes alone. However, this is fairly harmless, so I'd be tempted to say:

if(!String.prototype.endsWith) {
  String.prototype.endsWith = function (ending) {
    if(typeof ending !== 'string') return false;
    if(!ending) return true;
    return this.slice(-ending.length) === ending;
  };
}

Here we can skip checking origin, because we're adding the method to String itself. So if it's called, it's called on a string.

\$\endgroup\$
10
\$\begingroup\$

It's a bit ugly to get a substring (object creation and traversal of characters) and then compare it to the target (traversal of characters for a second time).

An alternative approach without object creation and fewer traversals would be using the indexOf method, something along the lines of:

function endsWith(source, target) {
    var start = source.length - target.length;
    return start >= 0 && source.indexOf(target, start) == start;
}
\$\endgroup\$
  • \$\begingroup\$ but .indexOf is (probably much) more heavy than a simple comparison \$\endgroup\$ – edc65 Dec 27 '15 at 21:06
  • \$\begingroup\$ @edc65 why would it be? \$\endgroup\$ – janos Dec 27 '15 at 21:07
  • \$\begingroup\$ Are you kidding? It's a totally different problem, there are specific algorithms developed to make it fast (en.wikipedia.org/wiki/…) \$\endgroup\$ – edc65 Dec 27 '15 at 21:10
  • 3
    \$\begingroup\$ @edc65 Given that indexOf is passed the starting position as an argument, it actually has to do very little work in this case. It basically reduces to a substring comparison. \$\endgroup\$ – JohannesD Dec 27 '15 at 21:52
  • 1
    \$\begingroup\$ Ok, sorry, I was wrong. When in doubt, benchmark it: jsperf.com/string-endswith/2 (the stupid version of indexOf is slow, but the your smart version is the fastest - not by far) \$\endgroup\$ – edc65 Dec 27 '15 at 22:00
7
\$\begingroup\$

You could remove the outermost parenthesis and change x * -1 to -x, making this:

return origin.substr(-target.length, target.length) === target;

If you don't like the -target.length you could use this version as well:

return origin.substr(origin.length - target.length, target.length) === target;

I tried to find an edge-case for what happened if the target was longer than origin, but it seemed to work correctly (or I just haven't tested enough).

\$\endgroup\$
  • 1
    \$\begingroup\$ From mdn: "If start is negative, substr() uses it as a character index from the end of the string. If start is negative and abs(start) is larger than the length of the string, substr() uses 0 as the start index. Note: the described handling of negative values of the start argument is not supported by Microsoft JScript." - I am guessing it is well-defined \$\endgroup\$ – Sumurai8 Dec 26 '15 at 19:12
  • \$\begingroup\$ If target is longer than origin you can also short-circuit to return false up front as a tiny optimisation. Although the === comparison should do that already. \$\endgroup\$ – millimoose Dec 27 '15 at 12:56
6
\$\begingroup\$

Is there a more idiomatic approach for writing such a basic function? Would slicing be more appropriate? The naming can probably be improved.

The other answers have already provided great information and great alternatives. However, I'm not too keen on some of their recommendations. For example, Flambino's solution is splitting a string and then doing string comparison; this can't be too fast, right?

When I thought of this code, I immediately thought of JavaScript's String.prototype.lastIndexOf; this gets the last index of a string in another string.

Using that method, you can now find if a string ends with another string with this:

str.lastIndexOf(end) == str.length - end.length

This will first find the ending string in the main string, and then it checks to see if it truly is the last part of the string by checking to see if it starts at the same index as would be needed for its length to take up the rest of the string.

\$\endgroup\$
6
\$\begingroup\$

You have implemented a function that will be a part of the ECMAScript 6 specification. Until all browsers support that, Mozilla Developer Network (MDN) recommends using the following snippet to add the function if it doesn't exists already:

if (!String.prototype.endsWith) {
  String.prototype.endsWith = function(searchString, position) {
      var subjectString = this.toString();
      if (typeof position !== 'number' || !isFinite(position) || Math.floor(position) !== position || position > subjectString.length) {
        position = subjectString.length;
      }
      position -= searchString.length;
      var lastIndex = subjectString.indexOf(searchString, position);
      return lastIndex !== -1 && lastIndex === position;
  };
}
\$\endgroup\$

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.