DEV Community

Cover image for I Re-Wrote These 10+ Single Lines of JavaScript Code, the Team Lead Praised the Code for Being Elegant
Rida F'kih
Rida F'kih

Posted on • Updated on

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

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 ‘I Wrote These 10+ Single Lines of JavaScript Code; the Team Lead Praised the Code for Being Elegant’.

One-liners are really cool challenges and very fun to make. I used to spend a lot of my time on codewars.com and codingame.com, which are two websites that facilitates little coding challenges which challenge your ability to write short code, or write code fast.

While code golf can serve as a fun engineering exercise, their lessons should rarely be used in the real world—and when they are—by no means should engineering managers be promoting developers to use this type of code in a codebase. When you’re in a collaborative environment, writing good code should be a priority. That means code that is readable, maintainable, and runs reliably.

Code golf is a type of recreational computer programming competition in which participants strive to achieve the shortest possible source code that solves a certain problem.

— Wikipedia, Code golf

Array Deduplication

Original Source Code

I’m not necessarily opposed to the common trick of removing duplicates from arrays by instantiating, and spreading a set, but due to the amount of times you’re indexing the array, it can be quite poor performance-wise.

Rewritten Source Code

On an array of 1,000,000 randomly generated numbers between 1 - 100, the original source code only performs at 12% of the speed that the rewritten version does. Since we use a map in the second function, we only have to iterate once over the array, and made it so that checking to see if we’ve already seen an element takes O(1) time.

The code eliminates “tricks,” and instead implores a well-known concept like hash maps, which may reduce the occurrence of inquiries by other developers on your code. The code also has well-named variables, with the function name removeDuplicateStrings defining the action that the function is doing, rather than an ambiguous name like uniqueArr, it also does not use excessively abbreviated words, making the code easier for non-native English speakers to understand.

Notice that the function is not documented. Excessive documentation can sometimes be a bad thing. If your variable names are well named, you may find your comments redundant, and that’s a good thing! If that happens, happily drop ‘em, and ship it!

If we’re absolutely dead-set on using spread operators and sets, or we want to remove duplicates of values that are not exclusively strings without writing bloated functionality, all we need to do is document our code, and ensure our variables are well-named.

Get Parameters from a URL and Convert Them to Object

Original Source Code

If there was a bug with this code, how excited would you be when you figured out this is the function you’d be working on? Odds are, not so much. Not to mention that re-building JSON objects with strings is something you want to avoid as much as possible, in this case that’d be quite easy.

Rewritten Source Code

In the rewritten code for this function, we use the convenient native JavaScript URL API which automatically invokes an instance of the URLSearchParams object for us, then we simply use the Object#fromEntries function on the resulting object! This prevents us from having to manually construct a JSON object by mingling and mangling strings.

Notice that we’ve provided a comment for this function! Despite the fact that I’ve included a specification for which type of parameters the function handles in the name of the function getURLParameters, it’s not necessarily clear what format the resulting object would be in.

Check Whether the Object is Empty

Original Source Code

Using ownKeys or Object.keys are both not very performant when checking if an object is empty.

Rewritten Source Code

This rewritten code is an old school method, and man has it passed the test of time! The re-written source code performs 20 times faster than the original method, and that’s only with two properties on an object. The performance boost comes from the fact that you no longer need to create arrays, copy over values, and index keys. You see one key and you’ve escaped the function!

It might not immediately be obvious why we’re iterating over the keys of the object, so we’ve left a short comment in there to ensure that any other developers know what’s going on.

Reverse the String

Original Source Code

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.

Rewritten Source Code

Splitting the chained methods across multiple lines is a personal preference of mine, as it allows me to scan the function paths rather than jumping through them, however if someone asked me to change it I wouldn’t make a fuss.

For fun, here is a more performant version.

Generate Random Hex

Original Source Code

Once again, not off to a bad start! I particularly like that we’re directly referencing the maximum hexadecimal value supported for non-alpha colours! Some minor cleanup, and we’ll be in business!

Rewritten Source Code

The only major change I made in this example is that I changed padEnd to padStart. The way this function works is by generating a random 24-bit integer, and then representing it in its hexadecimal format.

The minimum value we can get is 0 0x000000, with the highest value being 16,777,215 0xFFFFFF.

Let’s say our randomValue ended up being 255. Our hexValue would then be ff, and if we padded the end, our return value would be #ff0000, versus if we padded the beginning it would be #0000ff. If we then translate both these values back into integers, #ff0000 would actually represent the value 16,711,680, while #0000ff would represent 255. This means that if we pad the end rather than the beginning, we’d effectively get the wrong value.

In addition to this fact, with the first method, by padding the end rather than the beginning, #0000ff would actually be impossible to generate. Meaning there are plenty of missing values that could not possibly be generated. Specifically, every value between 0x000001 and 0x100000. That is 1,048,576 missing values from our function, but would also tip the odds in favour of all values that end with a 0, some more than others. For example, #100000 would be the result from 0x1, 0x10, 0x100, 0x1000, 0x10000, and of course 0x100000.

Conclusion

That took longer than expected, so I’ll be posting a second part to this blog post at a later date. I had a ton of fun refactoring this code. Would you have done something differently? Leave a comment on Medium or dev.to! I think this is a great learning opportunity for myself and others that might be in their code-golfing phase.

Want to read more of my stuff? You can check out my personal website. If you’d like to start a dialogue, feel free to tweet at me!

Top comments (52)

Collapse
 
paintedsky profile image
paintedsky

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

Collapse
 
rida profile image
Rida F'kih

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.

Collapse
 
codebytesfl profile image
codebytesfl

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.

Collapse
 
rida profile image
Rida F'kih • 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!

Collapse
 
codebytesfl profile image
codebytesfl

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.

Thread Thread
 
rida profile image
Rida F'kih

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

Collapse
 
mellen profile image
Matt Ellen • 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
Enter fullscreen mode Exit fullscreen mode
Collapse
 
rida profile image
Rida F'kih

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

Collapse
 
naveennamani profile image
naveennamani

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.

Collapse
 
rida profile image
Rida F'kih • 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("");
};
Enter fullscreen mode Exit fullscreen mode
Collapse
 
wadecodez profile image
Wade Zimmerman

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.

Collapse
 
rida profile image
Rida F'kih

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.

Collapse
 
brentdalling profile image
Brent Dalling

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

Collapse
 
rida profile image
Rida F'kih

Thank you so much! Totally with you here! 😄

Collapse
 
lexlohr profile image
Alex Lohr

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!

Collapse
 
rida profile image
Rida F'kih

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

Collapse
 
abhinav1217 profile image
Abhinav Kulshreshtha

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.

Collapse
 
rida profile image
Rida F'kih

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!

Collapse
 
offendingcommit profile image
Jonathan Irvin

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.

Collapse
 
rida profile image
Rida F'kih

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

Collapse
 
khuongduybui profile image
Duy K. Bui

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

Collapse
 
rida profile image
Rida F'kih • Edited

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

const reverseString = (string) =>
  string
    .split("")
    .reverse()
    .join("");
Enter fullscreen mode Exit fullscreen mode

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;

Collapse
 
khuongduybui profile image
Duy K. Bui

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

Thread Thread
 
rida profile image
Rida F'kih • 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!

Collapse
 
michahell profile image
Michael Trouw • 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.

Collapse
 
rida profile image
Rida F'kih

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!

Collapse
 
elliotmangini profile image
Elliot Mangini

Very cool, love it.

Collapse
 
rida profile image
Rida F'kih

Thank you for the kind words! 🥰

Collapse
 
darkwiiplayer profile image
𒎏Wii 🏳️‍⚧️

Array Deduplication

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

Collapse
 
rida profile image
Rida F'kih

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

Collapse
 
renaudham profile image
renaudham

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

Collapse
 
gilfewster profile image
Gil Fewster

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

Some comments may only be visible to logged-in visitors. Sign in to view all comments.