DEV Community

I Re-Wrote These 10+ Single Lines of JavaScript Code, the Team Lead Praised the Code for Being Elegant

Rida F'kih on October 20, 2022

I am the Team Lead, I praised the code. /s This post is in response to, but by no means a dig on the blog post written by Zard-x on Medium called ...

Read full post

paintedsky • Oct 20 '22

I would argue that your solutions are MORE elegant. I personally believe that there are more factors to "elegance" than simply length. Code that is extremely short but a dog's breakfast in terms of understanding and debugging is a non-starter for me. It's even worse if it's all that, and slow as well!

It's not like we're running out of lines in our text editors. Your solutions are beautiful, efficient, and readable. I know for a fact which ones I'd rather find in a codebase...

Rida F'kih • Oct 21 '22

Loving the dog's breakfast comment, completely agree. If I were to see any of those in a codebase, I'd dust off my backspace key and get to work refactoring those as fast as posdsible.

codebytesfl • Oct 24 '22

I like what you've done, but you're mainly just moving things to re-usable functions. This is generally a really nice approach and in my opinion makes things more more readable and usable. Rewriting to for loops versus built in javascript functions is a negative thing to me. Built in javascript methods and utilities such as map(), reverse(), split(), Set() and spread operators are much cleaner and most professional javascript developers can easily read and understand this.

Javascript is fast, and using a for loop for just performance reasons is negligible in most real world cases. If you have to process millions of objects in an array in client side javascript, you have bigger issues you need to be worrying about.

Rida F'kih • Oct 25 '22 • Edited

I can very well see how someone might very well come to that conclusion, my intention in the blog post was never to advise against using map, reverse, split, and Set simply based on their own merit or just because they're built-in functions. I do bring up valid use-cases for spreading a Set, and by no means forbid them as an absolute. I showcase that they absolutely can be implemented fairly, and went on to specifically highlight a situation in which they actually prevail over other methods.

Specifically in the case for the Set, for example, I mention that if you want to go with that implementation, it wouldn't hurt to leave a comment explaining how it works for developers that might not have come across a Set-spread before. Simple as that! In terms of the string reversal, which I'd guess is the one that might lead people to believe I'm advising from built-in functions for no good reason, I did say in the blog post...

In this case, we’re not off to a bad start! We could use a for-loop to get an 80% performance boost on short strings, and we might consider implementing that method if we’re reversing some chunky strings often, but in the case of the odd reversal on lightweight strings, we might just be better off with this functionality.

...which is to say, unless we're doing some odd massive string-reversals, we should be just fine with the original code, and I simply provided the more performant version "for fun," as stated in the article.

While I do touch on speed (which I think is important not to neglect) especially when working with heavy frontend libraries such as React which don't always provide a lot of room for error for newer developers, major takeaways and talking points are the naming of variables, length of lines, scan-ability of code, legibility of code, actual methods, genuine mistakes in the code, and to not prioritize code shortness over performance. By no means was the takeaway supposed to be "stop using built-in methods and start using for loops," for if that were the case, I'd be a hypocrite.

Please also note that this is a blog post about JavaScript, not exclusive to frontend development in particular, so to critique because frontend feels a little disingenuous. Regardless, I hope you were able to enjoy the article somewhat.

EDIT: Just a few minutes after my response, giving this reply a once-over I apologize if I came off aggressive or accusatory! For full posterity, I 100% agree that the changes I made in these refactor are not to be a catch-all, not always applicable, and not always necessary. As a developer, granted the majority working on the codebase agree with a style of coding, I'm game! Cheers!

codebytesfl • Oct 25 '22

Great clarification. My main point is your method of splitting code off into re-usable functions I think was the single biggest improvement to readability and elegance. I do it all the time, and I see it done all the time in enterprise apps. I think this idea should be highlighted more in general.

Rida F'kih • Oct 25 '22

You're right, that is a point I completely neglected! Thank you so much for the feedback! ❣️

Matt Ellen • Oct 21 '22 • Edited

Interesting article. I'm always in favour of more readable code!

Arguably, your implementation of isObjectEmpty is wrong. The original checks the constructor, too, limiting the types of object that can be deemed empty.

function EmptyObject()
{
}

const empty = new EmptyObject();

const isObjectEmpty = obj => Reflect.ownKeys(obj).length === 0 && obj.constructor === Object;

const isObjectEmptyNew = (object) => {
  // Iterates over the keys of an object, if
  // any exist, return false.
  for (_ in object) return false;
  return true;
};

console.log('CURRENT the instance is empty:', isObjectEmpty(empty)); //false

console.log('NEW the instance is empty:', isObjectEmptyNew(empty)); //true

Rida F'kih • Oct 21 '22

That's a great point! I'll go ahead and throw a guard clause in there. c: Thanks for your comment!

naveennamani • Oct 21 '22

I don't see how reverseString function using for loop is more performant. Because strings are immutable and everytime you do str+=c you're creating a new string. For longer strings this may not be performance at all.

Other solutions are quite good especially empty object detection.

Rida F'kih • Oct 21 '22 • Edited

First of all, thank you! This one was definitely my most uncertain of the bunch both in terms of keeping clean and efficiency. I based the claim off of benchmarks ran on jsben.ch, and only cited performance claims if there was a large enough discrepancy.

On short strings, the improvement can be upwards of 80% between the two, while on longer strings there's diminishing returns until it reduces down to about a 20% increase in performance.

In the case of this specific example, I ran the test on the string "garbanzo beans" as a shortString, and the entirety of the DreamWorks Animation 2007 "The Bee Movie" script repeated ten, twenty-five, and fifty times as longString. The performance appeared as advised above.

I did fiddle with another alternative I was going to include for the article, but decided against it seeing as I wasn't getting the performance boost I expected, as well as the fact that it was considerably less readable than the alternatives. Here is a draft of that function if you're curious.

const reverseE = (string) => {
  const { length } = string;
  if (length === 1) return string;

  const iterations = Math.floor(length / 2);
  const reversed = [];

  for (let i = 0; i <= iterations; i++) {
    reversed[i] = string[length - i - 1];
    reversed[length - i - 1] = string[i];
  }

  return reversed.join("");
};

Wade Zimmerman • Oct 22 '22

It's important to remember that refactoring does nothing for the end-user. If you are refactoring for the sake of refactoring, you will probably upset your boss. It's safer to refactor as you go. And try to limit refactoring to code that your already modifying.

Rida F'kih • Oct 22 '22

This is a great point, Wade, and in my opinion would make a very good and potentially controversial article! Albeit, I completely agree with you.

Brent Dalling • Oct 20 '22

Well done. The code you wrote seems to be self documenting and indeed takes into account time complexity. The result is nice, readable, and performant code compared the languages "magic tricks".

Rida F'kih • Oct 21 '22

Thank you so much! Totally with you here! 😄

Alex Lohr • Oct 22 '22

After having seen a post with a similar click-bait-y title yet with content obviously written by an incompetent novice, I wasn't expecting much from his article, but against my expectations, you actually delivered. Nice one!

Rida F'kih • Oct 22 '22

Thanks Alex! I can't say I didn't shutter at my own title after writing it, but I felt like it was a good starting point from the other blog post, and makes it a little fun. I still cringe every time I see it, though. 😆

Abhinav Kulshreshtha • Oct 23 '22

Finally a good article. You clearly explained that it is not just the question of readability, but also about the performance benefits of techniques, you also showed that being clever doesn't equates to being efficient.

We have a well defined Util library for our project, with similar old school but performant functions. Every year, with every new batch of recruits, comes at least one such fresher who thinks they know better, or that showing these one liner tricks will make them stand out better than rest of the batch. Few of them even had balls to tell me that I am old and maybe I haven't kept myself updated to latest coding practices.

Always micro-optimizing may be bad, but there are many frequent use cases where micro-optimization may aggregate into quite a load of performance improvement.

Rida F'kih • Oct 24 '22

Ah, to claim one hasn't kept with the times! That's bold! I'm really glad you enjoyed the article, and thank you for the kind words. I agree that micro-optimizing can be bad, spending precious man-hours for a 15ms performance boost isn't a great use of time. You definitely get a lot more "bang for your buck" when it comes to refactoring for performance with libraries such as React, as there's a lot of classic and easy-to-make mistakes that can be pretty detrimental.

I think that if anyone would like to propose performance-based refactoring, quantifiable metrics, expectations, and cost-savings would be a great way to back up the claim that it's needed!

Thank you!

Jonathan Irvin • Oct 27 '22

I'm a big fan of "human read code, machines interpret."

That being said, your refactors are clean where your comments are, but call me old-fashioned, I prefer JSDocs. That way you have hints where your functions are being used and you can expect the correct output.

As far as performance goes, ¯_(ツ)_/¯ . I stand by readability-first because performance only lasts as long as the person who wrote it remembers :). The moment they leave, it becomes a prime target for refactoring into something that's more common-knowledge.

Rida F'kih • Oct 28 '22

I also prefer JSDocs, but for this article acknowledged I can be the odd-one-out here and gave it a bit more familiarity.

Duy K. Bui • Oct 23 '22

On the string reverse what makes you insist on having an explicit return statement?

Rida F'kih • Oct 23 '22 • Edited

I wouldn't insist on it, I think implied would be just fine.

const reverseString = (string) =>
  string
    .split("")
    .reverse()
    .join("");

Main notable changes on the original "rewritten" version were changing str to string, changing reverse to reverseString, and splitting across multiple lines. However, if someone still wrote it on one line, it'd probably be fine granted it passed the linter configuration, and I wouldn't mind leaving everything on one line either, just a preference honestly. c;

Duy K. Bui • Oct 23 '22

Makes sense. Thanks for the clarification. I thought there was something about implicit returning to abstain from that I wasn't aware of.

Rida F'kih • Oct 23 '22 • Edited

Nope! You're (imo) in the clear! 😎 As long as you're not refactoring what would otherwise be a well-written function to keep it in one line so you can have it be implicit, I'd be happy!

Michael Trouw • Oct 23 '22 • Edited

I agree with all your readability preferences and explanations. However when it comes to performance, I feel that 9 out of 10 times performance on frontend is a non-issue: FE should only be a tiny layer of UI UX on top of a much more complex and performance-sensitive system behind it.
I actually prefer some of the lesser performant solutions in terms of readability.
When performance starts to become a bottleneck, I think the right question to ask is: why are we even doing X on the frontend if it is so performance-sensitive?
Does it even make sense to do it in the frontend? Why not have a super fast Rust backend perform this X for us and then return us the result instead.

Rida F'kih • Oct 23 '22

Totally, two of my favourite examples for readability being sacrificed for performance were the second reverseString function, and the isObjectEmpty function. A basic refactor of the original two (better named variables, removing obnoxiously long lines, etc.) would make it a passable solution in a frontend codebase.

With frontend performance in most cases, since the code is running on the users machine it totally makes sense to de-prioritize performance, on the backend with NodeJS depending on your setup (single instance, non-serverless, etc.), and depending on the actual task you're running, it might make sense to prioritize performance in some instances. One instance where it might be useful to prioritize performance on the frontend however, is when working with WebGL wrapping libraries (ie. ThreeJS).

Writing a blog post as a complete catch-all is impossible, but there is definitely a small art to deciding when to prioritize which.

Thank you for your comment!

Elliot Mangini • Oct 20 '22

Very cool, love it.

Rida F'kih • Oct 21 '22

Thank you for the kind words! 🥰

𒎏Wii 🏳️‍⚧️ • Oct 25 '22

Array Deduplication

You can make your solution even faster by using a set instead of an object to remember the known values :D

Rida F'kih • Oct 25 '22

Do you have an example? Would love to see! Please list browser engine and environment as well! Would be much appreciated.

renaudham • Oct 24 '22

I like all this.
In terms of performance I would have said also that in case of heavy arrays, classical old for loop is usually more performant than array functions map forEach etc...

Gil Fewster • Oct 24 '22

Wonderful to see posts like this which emphasise the value of performant, legible code.

Yongchang He • Oct 24 '22

Thank you for sharing!

Key • Oct 23 '22

Loved this. Fresh Rida, thanks

Rida F'kih • Oct 23 '22

Thank you! ❤️‍🔥

bencun • Oct 22 '22

That original post was just... Nonsense. I appreciate your approach to dismantling it.

Rida F'kih • Oct 23 '22

Thanks! The original post was definitely an interesting read, can't say I didn't have a short code phase myself when I was a beginner, but to write a blog post self-praising the code's elegance was a much more bold move, I just hope the number of beginners that took a lesson from that thread is very low. 🤣

When writing on either Dev.to, Medium, and other blogging platforms, you got to be ready for everyone to look through your code with the finest tooth comb you've seen in your life. Better be ready for that! c:

WZRD • Oct 21 '22

Great stuff. Definitely learned a lot. Best to just stick to the basics it seems. People get creative with their solutions, but forget that many methods aren't performant.

Jesper Krejberg Petersen • Oct 22 '22

Back in 'the good old days' a rule of thumb said:
'You can have it small or fast, not both.'
That's still true and goes for both code and data structures.

Duy K. Bui • Oct 23 '22

You can have it cheap or fast, too. :D

Arun Bohra • Oct 23 '22

I like the rewritten codes.
Just want to know how do you measure the speed of your code?

Rida F'kih • Oct 24 '22

Thank you! For this article, I used jsben.ch!

Kacper Turowski • Oct 26 '22 • Edited

Lord! Yes! Thank you!

I really don't understand why some people think we should just cram as much code in single line as it's humanly possible. Is it because people tend to say that files should be around 500 lines long and not more?

Why does someone think that a 2000 character long line is better? All that my ADHD brain sees is

const getParameters = LoremipsumdolorsitametconsecteturadipiscingelitSedvariusmetuspurusvelpulvinarleosodaleseuVestibulumsagittiselitquisleoviverramalesuadaMorbilaoreetposuereloremvitaeposuereSedpellentesquearcunontinciduntgravidaMorbisedorciipsumFuscejustonibhullamcorperattristiquequisaccumsaninaugueMaurismattiseuismoderatquisdapibusNullamvitaenequedolorSedquiseuismodmetusvelluctusmetusSedvolutpatidnislquisefficiturCurabiturlacinianisietrutrumviverraestnuncconvallisrisusaimperdietmagnaestnecmaurisPellentesquequisligulanecurnablanditiaculisInlectusduitristiquesitametduisitametaccumsaneleifendlacus;

Working code is not code golf. Heck, even katas are not code golf.

Jason Van Malder • Oct 25 '22 • Edited

I just made a little benchmark: Using Set() vs not using Set() to remove duplicates from an array.

Remove duplicates from array (Set VS loop)

Set: 16 ms
Loop: 4333 ms

Source:

const arr = [...Array(1000000).keys()].map(() => Math.floor(Math.random() * 10000));

// Remove duplicates from array with Set
const removeDuplicates = (arr: number[]): number[] => {
  return [...new Set(arr)];
};

const firstStart = Date.now();

removeDuplicates(arr);

const firstEnd = Date.now();

// Remove duplicates from array without Set
const removeDuplicates2 = (arr: number[]): number[] => {
  const result: number[] = [];
  arr.forEach((item) => {
    if (!result.includes(item)) {
      result.push(item);
    }
  });
  return result;
};

const secondStart = Date.now();

removeDuplicates2(arr);

const secondEnd = Date.now();

console.log(`Remove duplicates from array (Set VS loop)`);
console.log(`---------------------------------`);
console.log(`Set: ${firstEnd - firstStart} ms`);
console.log(`Loop: ${secondEnd - secondStart} ms`);

Rida F'kih • Oct 25 '22

Thanks for your reply! You seem to have built a completely different solution than that mentioned in the article, the reason the function you've created is so slow is because you're iterating over each element in the array at Array#forEach, then on each item in the array you're iterating on the array again Array#includes, this effectively means that your function runs in O(n^2) time, versus the "performant" function in the article running in O(n) time.

Please note that it's not advisable to run benchmarks this way, it's best to run them using a benchmarking tool in an isolated environment.

Anyways, here is the portion of your script re-written with the function listed in the article in mind, where the nested iteration is not present.

const arrayOfRandomValues = [...Array(1000000).keys()].map(() =>
  Math.floor(Math.random() * 10000)
);

const removeDuplicates = (array) => {
  const uniqueValues = [];
  const seenMap = {};

  for (const item of array) {
    if (seenMap[item]) continue;
    seenMap[item] = true;
    uniqueValues.push(item);
  }

  return uniqueValues;
};

const start = performance.now();
removeDuplicates(arrayOfRandomValues);
const end = performance.now();

console.log("Run for-loop duplicate removal function.");
console.log(`${end - start}ms`);

The resulting output was 14.339...ms on my personal machine, but once again this is not an isolated environment.

Hutch • Dec 2 '22

The new getRandomHexColor function has a slight difference: it doesn't prepend the string with a #.

Interesting that the new string reversal function is faster, since, naively, it would create n strings, where n is the number of characters in the string. The engine must be doing some optimizing in the background, which you wouldn't know without benchmarking (or digging into its implementation).

nogou21 • Oct 25 '22

i'm having mixed emotions about the this article, i agree with part of it but for the string reverse is a big 'NO' for me . the solution works better only for very short strings , i think the original article made good use of native string methods.

Rida F'kih • Oct 25 '22 • Edited

In my benchmarks, I ran the two functions on two strings. A very short string, and a series of increasingly excessively long strings. You're right, there is diminishing returns of performance and in my benchmarks it bottomed out to a 20% increase in performance rather than the 80% you would see on shorter strings.

Remember, these refactors are not absolute, especially pertaining to the string reversal. The more performant version was, as mentioned in the article, only included for fun, and I do outline there is absolutely nothing wrong with using the native methods.

As developers, I know we get excited about the meat of the code, but as mentioned to another commenter, there were other equally important aspects to the article than performance.

"major takeaways and talking points are the naming of variables, length of lines, scan-ability of code, legibility of code, actual methods, genuine mistakes in the code, and to not prioritize code shortness over performance."

In the context of the string reversal specifically, the--albeit subjective--improvements I made to the original method were making the function name, and variable names more clear, splitting the chained method calls across multiple lines to make the function more skimmable, outlining that the latter point is completely optional and not anything to make a fuss over. I then provided the more performant version for fun, as outlined!

devtolove • Oct 25 '22

incredibly