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!

Comments

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

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

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

[Rida F'kih]

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]

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]

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

[Matt Ellen-Tsivintzeli]

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]

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

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

[Rida F'kih]

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]

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]

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]

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]

Thank you so much! Totally with you here! 😄

[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!

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

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

[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!

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

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

[Duy K. Bui]

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

[Rida F'kih]

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]

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]

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!