9
\$\begingroup\$

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?

\$\endgroup\$
  • 8
    \$\begingroup\$ How much is this profession being dumbed down that parts[parts.length - 1] is considered "hard to read"? \$\endgroup\$ – Will Sep 11 at 0:13
  • 1
    \$\begingroup\$ "performance does not matter if you use big frameworks like React for example" - they have a point so: when you replace pile of poorly written code that searches dom to manually replace value one-by-one with big heavily optimized framework like Reach you have a lot of free room to write slower code till you match your original slow version... \$\endgroup\$ – Alexei Levenkov Sep 11 at 1:05
  • 1
    \$\begingroup\$ @Will It looks like the OP is saying the second method is hard to read, not the parts[parts.length - 1] part. \$\endgroup\$ – Kodos Johnson Sep 11 at 1:46
  • 1
    \$\begingroup\$ @KodosJohnson Yes, and I agree with the OP, but some of the answers below and presumably the person responsible for OP's PR do imply that parts[parts.length - 1] is somehow hard to read. \$\endgroup\$ – Will Sep 11 at 3:08
  • 1
    \$\begingroup\$ @12Me21 Although it is technically the first element in the array, how is const [last] = array in any way difficult to read, let alone considered an obfuscation? Anyone familiar with object destructuring (so common in React now) ought to at least have some basic understanding of its cousin, array destructuring. I think more important to the code review the OP did was if that change was even necessary to begin with as it appears to have been a completely superfluous implementation change on the coder's part. "Just because" isn't really a valid reason to change working production code. \$\endgroup\$ – Drew Reese Sep 12 at 6:44
13
\$\begingroup\$

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

| improve this answer | |
\$\endgroup\$
  • 2
    \$\begingroup\$ Perhaps not so intuitive but a great solution. \$\endgroup\$ – Miguel Avila Sep 10 at 13:52
  • 3
    \$\begingroup\$ @WilliMentzel Sure, if the array needs to be used later, then mutation via .pop earlier could well cease to be the right approach. If that was the situation, I'd use the index method instead, it's easier to read than the .reverse() / destructuring method. \$\endgroup\$ – CertainPerformance Sep 10 at 14:13
  • 3
    \$\begingroup\$ @Willi It depends how much of a stickler you are on a functional paradigm. Mutation is bad when it makes the code harder to read than the alternative, but I don't consider it a cardinal sin; sometimes aiming to be as functional as possible makes the code more confusing than the alternative. For example, when you want to (eventually) do something with a deeply nested object via a property string, splitting by . and then popping off the last item to have [(1) array of all properties but the last (2) the last property string] is what I'd do. \$\endgroup\$ – CertainPerformance Sep 10 at 14:25
  • 3
    \$\begingroup\$ YMMV; it's up to you, the standards of your codebase, and the situation. \$\endgroup\$ – CertainPerformance Sep 10 at 14:25
  • 2
    \$\begingroup\$ @WilliMentzel - In this case the nobody can use the array generated because it is not saved in any variable. However, if you have the array stored in a variable then do x.slice().pop(). There is zero arguments against using pop \$\endgroup\$ – slebetman Sep 11 at 3:00
9
\$\begingroup\$

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!

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

  1. 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);
| improve this answer | |
\$\endgroup\$
  • 3
    \$\begingroup\$ array[array.length-1] is idiomatic in so many languages that I think it would make sense to consider it as one step. I imagine most people would immediately recognize that as "get the last item in the array" \$\endgroup\$ – 12Me21 Sep 11 at 17:12
  • \$\begingroup\$ Steps, as I describe in the post, disregard an individual's ability to chunk understood steps together. The focus is on what the person reading might have to go through, at a bare minimum, to read the code. I do not think it wise to assume everyone reading the code will have a predetermined background, hence my decision to not chunk those into one step. \$\endgroup\$ – santanaG Sep 11 at 21:17
5
\$\begingroup\$

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)

| improve this answer | |
\$\endgroup\$
4
\$\begingroup\$

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

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ I know that nobody asked, but as a C++ programmer, this is the only answer that doesn't sound crazy to me. I would never even consider tokenizing a string and then reversing an entire array just to get the last element in a string. \$\endgroup\$ – JPhi1618 Sep 17 at 13:26
  • \$\begingroup\$ @JPhi1618 Exactly this. Years ago I agreed about not caring too much because JS was used mostly for fluffing some bits on the browser. However, a substantial number of backends today use JS. I hope that those developers are aware of what the code they write is actually doing rather than just its return value. \$\endgroup\$ – Bernardo Sulzbach Sep 24 at 1: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.