Sign up ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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.

share|improve this question
    
Pick one: stackoverflow.com/questions/280634/endswith-in-javascript It definitely should be an extension of the String prototype. – Jeroen Vannevel 4 hours ago
1  
@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? – Mast 4 hours ago
    
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. – Jeroen Vannevel 3 hours ago

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 result is due to essentially checking slice(0), which just returns 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. This is fairly harmless, though, 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.

share|improve this answer

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).

share|improve this answer
1  
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 – Sumurai8 2 hours ago

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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