Returning the last segment of a split string

Question

I recently reviewed a PR and saw the following.

const parts = someString.split('.');
return parts[parts.length - 1];

was changed to

const [last] = someString.split('.').reverse();
return last;

I commented that I am against the change for the reasons that it is harder to read and not performant.

The answer was that it is only hard to read, because I am not used to it and that performance does not matter if you use big frameworks like React for example.

Who is right and why?

Answer 1

I'm not entirely enthusiastic about either. The parts[parts.length - 1] using manual index lookup and subtraction may well take a moment to recognize, "Oh, this is getting the last element of the array". The second, using .reverse() followed by destructuring of the first item also could take a moment to think about before you understand what exactly it's doing.

As an alternative, consider using .pop() instead, I think it's more intuitive than both:

return someString
  .split('.')
  .pop();

The only (tiny) downside is that the above isn't functional, since it mutates the split array - but that's OK, since the array isn't being used anywhere else.

It's true that performance for something like this isn't really something to worry about if this is used in a larger project - what matters more is if it's easier to understand what the code is doing.

Remember that if something isn't immediately intuitive, that's what comments are for, though all of these cases are self-documenting enough that it's not needed.

Another way to make it easier to recognize what's going on would be to put this into a function with a descriptive name, such as getLastProperty (if the input is meant to show a path to a nested property lookup, for example - name it as appropriate for how it's meant to be used).

Answer 2

tl;dr at the end.

In terms of 'performance', js engines are pretty well optimized. For something like this, performance should not be part of your argument against a given piece of code. In terms of readability, they are very similar so either works. There are better ways to think through this though.

When it comes to PRs I would ponder on a few questions:

1. What does the PR code in question actually change?

Between the two snippets of code, there are no changes to the API of the calling code and no implementation changes to accommodate any edge cases. They work the same way and there are no visible upsides. So why change it? Original takes it here. DFWAB!

2. Does the code add/remove unnecessary steps?

const parts = someString.split('.');
return parts[parts.length - 1];

This reads as:

1. Split someString into an ('.' character delimited) array
2. Label (step 1) parts
3. Take parts's length property and subtract 1
4. Retrieve parts's element at index (step 3)
5. return (step 4)

const [last] = someString.split('.').reverse();
return last;

This reads as:

1. Split someString into an ('.' delimited) array
2. reverse the elements in (step 1)
3. Label the first element in (step 2) last
4. return last

Here the PR code changes the approach and removes a step. Is it necessary though? This is more left to opinion, and I am sure you will get opinions for/against both. IMHO "split an array then reverse that array" are simpler to reason about because it is declarative. I can see a string being split into pieces in my head and then reverse-ing the original order so the first element becomes the last (supported by the const label last). In the original, I have to reason about arrays, their properties, and why I have to access its length at all. I say that the PR code takes this. Not by much though.

3. What is the intention of the code?

This code's intention regardless of implementation is to take a string and give back a substring starting one character after the last '.' in the string. Both implementations perform acrobatics just to pull this off. The main fault in terms of implementation is 'why do we have to turn the string into an array' when the string possesses all the necessary methods?

Here is an example that beats out both cases and how it performs in these metrics:

return someString.slice(someString.lastIndexOf('.') + 1);

1. No changes from the original code BUT adds clear benefits. (win)
2. Removes unnecessary/takes fewer steps and is more declarative. (win)
3. States the true intention of the code clearly. (huge win)

BONUS: IT'S A ONE LINER! WOOHOO!

Now I know people tend to not see this as a benefit but I think shorter code with higher stats (as described above) is better than more code performing equally or worse.

The code reads as:

1. Take someString and find the lastIndexOf '.'
2. Add 1 to (step 1)
3. slice someString starting at the index (step 2)
4. return (step 3)

Just a small note: if I were certain that this was all the code in the calling function I would turn it into someString => someString.slice(someString.lastIndexOf('.') + 1) and eliminate step 4 since the return is implicit and the function can be seen as the result of the first three steps.

In terms of intention, this line is literally saying (as the steps also describe): "Slice this string starting at the index following the last index of '.'. It's just clear.

tl;dr

Neither one is 'right' because performance is irrelevant and both snippets are fairly similar in readability. Instead of 'readability' (whatever that means, highly subjective) look at these metrics when it comes to PRs:

1. Does the PR actually implement changes? (original wins)
2. Does the PR add/remove unnecessary STEPS? (PR wins)
3. Does the pull request make the intention of the code clearer? (tied loss)

A better change which beats the other two in all categories (for reference) could be something like this:

return someString.slice(someString.lastIndexOf('.') + 1);

Answer 3

Well, it depends if the performance is an objective or it isn't. Now, I always would prefer good performance; now respect to the person who said "performance does not matter if you use big frameworks like React" oh, you don't really mean that, is Facebook that slow ?(Facebook uses React) do you know what implies to have a slow social network? Slow transactional operations not because the backend isn't optimized but the front? It is less users because they're boored waiting your app to response and switch to another media.

Now, the second code fragment

const [last] = someString.split('.').reverse();
return last;

is slower and does not exhibit the advantages of the language you are working with so why bother to even write it, if it's redundant?

I mean, why would you need to reverse a substring to return the first character on it? (after reversed)

You clearly can do

const parts = someString.split('.');
return parts[parts.length - 1];

and that doesn't cost more time and is descriptive.

The features of any language are there not to create "smart looking code" but to create more effective and efficient code.

---

Note, as CertainPerformance mentioned

return someString.split('.').pop();

is a great solution, easier to read, write and virtually takes the same time that the first fragment. (virtually, because internally those are different operations)

Answer 4

Here's my take:

return someString.substring(someString.lastIndexOf('.') + 1);

The readability issue could be argued either way, it's really personal preference. I don't think the change in the PR makes the code materially better or easier to read.

To hear 'performance doesn't matter' just makes me queasy. In your application it might not have much of an effect, but why go out of your way to make code slower if it doesn't really make the code much easier to read.

const ITERATIONS = 350000;
const TEST_STRING = 'a.b.c.d.e.f.g.h.i.j.k.l.m.n.o.p.q.r.s.t.u.v.w.x.y.z';
console.log('------------------------------ START ------------------------------')

function time(s, fn) {
    const start = performance.now();
  for (let i = 0; i < ITERATIONS; i++) {
    fn(TEST_STRING);
  }
  const end = performance.now();
  const time = end - start;
  console.log(`${s}: ${time}`);
}

time('Length-1', s => { const arr = s.split('.'); return arr[arr.length - 1]; });
time('Reverse', s => { const [last] = s.split('.').reverse('.'); return last; });
time('lastIndexOf', s => s.substring(s.lastIndexOf('.') + 1));