3
\$\begingroup\$

I have created a function which randomly sets styles using CSS selectors. Right now it returns a random item from an array. The program also works with shorthand properties (such as font).

Could my code be improved and run more efficiently?

const setRandomStyles = (obj) => {
  console.time("Executed in ");
  Object.entries(obj).forEach(([target, styles]) =>
    document
      .querySelectorAll(target)
      .forEach((element) =>
        Object.assign(
          element.style,
          Object.fromEntries(
            Object.entries(styles).map(([key, values]) => [
              key,
              Array.isArray(values[0])
                ? values.map((e) => e[~~(Math.random() * e.length)]).join(" ")
                : values[~~(Math.random() * values.length)],
            ])
          )
        )
      )
  );
  console.timeEnd("Executed in ");
};

setRandomStyles({
  "*": {
    color: ["red", "orange", "yellow", "green", "blue", "violet"],
    backgroundColor: ["yellow", "green", "blue", "violet"],
    font: [["italic", "bold"], [...Array(40).keys()].map((i) => i + "px"), ["cursive", "sans-serif", "consolas"]],
  },
});

You can also propose better code to set random styles in js. Thanks!

\$\endgroup\$
5
  • \$\begingroup\$ How many browsers are you targeting? MDN says that "some older browsers have not implemented NodeList.forEach()". \$\endgroup\$
    – Teepeemm
    Mar 23, 2022 at 22:28
  • \$\begingroup\$ @Teepeemm, I go for the fastest and most "modern" solution (so using ES6) \$\endgroup\$
    – Gibberish
    Mar 24, 2022 at 9:48
  • \$\begingroup\$ This solution is very nice. The only thing I might consider is pulling ~~(Math.random() * e.length into a utility function randElement(arr), though the phrase is so short the duplication hardly matters. \$\endgroup\$
    – Jonah
    Mar 27, 2022 at 6:03
  • \$\begingroup\$ @Jonah so my code is already optimized and does not need any improvements ? (appart from the suggestion you mentioned) \$\endgroup\$
    – Gibberish
    Mar 27, 2022 at 9:24
  • \$\begingroup\$ From a readability perspective, yes it is imo. I did not analyze it from a performance perspective. \$\endgroup\$
    – Jonah
    Mar 27, 2022 at 13:50

1 Answer 1

0
+50
\$\begingroup\$

Code Style

This code looks very identical to the code in your recent question Download entire localStorage as file. Like I mentioned in my answer there it does look a bit "pyramid shaped". I see this basic pattern

function
  Object.entries().forEach
    Object.assign
      Object.fromEntries
        Object.entries().map
          map || item

That is 6+ levels of indentation! It is a good thing you are only using two spaces instead of four (or even worse- tabs) - otherwise the line lengths would be quite a bit longer.

Still it doesn't seem totally unreadable.

Could my code be improved and run more efficiently?

Efficiency

One could perhaps improve efficiency by switching to basic for loops. While it strays from the functional programming techniques, the example snippet below uses for...of loops, which still uses an iterator so it isn't as fast as a regular for loop but that will likely not be noticeable for small DOM trees. It also eliminates the need to call Object.assign() and Object.fromEntries() as it assigns to the style directly in each iteration. While it contains the same complexity (i.e. two loops nested within a loop) the indentation levels are fewer and there are fewer functions executed per iteration.

const setRandomStyles = obj => {
  console.time("Executed in ");
  for (const [target, styles] of Object.entries(obj)) {
    for (const element of document.querySelectorAll(target)) {
      for (const [key, values] of Object.entries(styles)) {
        element.style[key] = Array.isArray(values[0])
          ? values.map((e) => e[~~(Math.random() * e.length)]).join(" ")
          : values[~~(Math.random() * values.length)];
      }
    }
  }
  console.timeEnd("Executed in ");
};

setRandomStyles({
  "*": {
    color: ["red", "orange", "yellow", "green", "blue", "violet"],
    backgroundColor: ["yellow", "green", "blue", "violet"],
    font: [["italic", "bold"], [...Array(40).keys()].map((i) => i + "px"), ["cursive", "sans-serif", "consolas"]],
  },
});

Arrow function syntax simplification

While it may not be a big difference, "Multiple params require parentheses"1 but if an arrow function has just one parameter then the parentheses can be omitted (as was done in the snippet above).

For Example:

const setRandomStyles = (obj) => {

can be simplified to:

const setRandomStyles = obj => {

And

(e) => e[~~(Math.random() * e.length)]

can be simplified to:

e => e[~~(Math.random() * e.length)]

Simplify selectors

While style is a global attribute it may just be for demonstration purposes, the selector * applies to all elements within the HTML page - including tags that likely don't need to have style attributes - e.g. <html>, <head>, <link>, <meta>, etc..

For example, I ran the code on a sample HTML page on jsbin.com and it showed this HTML (simplified and formatted for brevity):

<html style="color: red; background-color: violet; font: bold 6px sans-serif;">
  <head style="color: green; background-color: blue; font: italic 3px sans-serif;">
    <script style="color: orange; background-color: violet; font: italic 21px consolas;"> /* script source */</script>
    <meta charset="utf-8" style="color: violet; background-color: violet; font: bold 29px sans-serif;">
    <meta name="viewport" content="width=device-width" style="color: violet; background-color: violet; font: bold 26px consolas;">
    <title style="color: orange; background-color: yellow; font: bold 6px cursive;">JS Bin</title>
    <style id="jsbin-css" style="color: violet; background-color: blue; font: bold 20px cursive;">
    </style>
  </head>
  <body style="color: blue; background-color: yellow; font: italic 14px consolas;">
    sample text
    <div style="color: violet; background-color: yellow; font: bold 22px sans-serif;">Hey there dude</div>
    <script style="color: green; background-color: violet; font: italic 8px cursive;">/* script source */ </script>
  </body>
</html>
\$\endgroup\$
5
  • \$\begingroup\$ Thanks although try pasting your code in the dev console of this page and you will get Uncaught TypeError: Cannot set properties of undefined (setting 'color'). Why is that happening ? \$\endgroup\$
    – Gibberish
    Mar 30, 2022 at 12:15
  • \$\begingroup\$ Apparently a similar error happens with the original code: Uncaught TypeError: Cannot convert undefined or null to object. It appears to be related to the MathJax elements on the CR pages. Run document.querySelector('math') and it should reveal an element <math xmlns="http://www.w3.org/1998/Math/MathML"></math>. I'm not really familliar with that namespace but it seems such an element has no support for style in its attributes. \$\endgroup\$ Mar 30, 2022 at 16:03
  • \$\begingroup\$ the arrow function simplification isnt good practice imo. Since it introduces inconsistency when you have more than one arg. IMO, its best to stay consistent and always use parents. It is also more clear and readable that way, as it conveys the nature of it better. \$\endgroup\$
    – The Fool
    Mar 31, 2022 at 7:00
  • \$\begingroup\$ Just one more thing, do you think there could be a better way to set random styles using js? For example in the way the function is used. \$\endgroup\$
    – Gibberish
    Mar 31, 2022 at 9:12
  • \$\begingroup\$ @Gibberish there likely could be a better way. Also - there are ways to get random colors - e.g. generating hex values instead of using a finite list of color names \$\endgroup\$ Mar 31, 2022 at 17:19

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.