Is there a way to rewrite this basic function so I'm not repeating the same lines twice?

user_14495368 0
Viewed 120

const body = document.querySelector( `body` );

function getRandomNumber( max ) {
  return Math.floor( Math.random() * max )
}
function getRandomRGB() {
  const colors = [];
  
  for( let i = 0; i < 3; i++ ) {
    const  n = getRandomNumber( 100 + 1 );
    
    colors.push( n );
  }
  return `rgb(${ colors.join( '%,' )}%)`; 
}
function changeRGB() {
  const divs = body.querySelectorAll( `div` );
  
  for( let i = 0; i < divs.length; i++ ) {
    divs[ i ].style.backgroundColor = getRandomRGB();
    divs[ i ].style.transitionDuration = `${ getRandomNumber( 10 ) + 1 }s`;
  }
}

for( let i = 0; i < 25; i++ ) {
  const div = document.createElement( `div` );
  
  div.style.backgroundColor = getRandomRGB();
  div.style.transitionDuration = `${ getRandomNumber( 10 ) + 1 }s`;
  body.appendChild( div ); 
}

setInterval( changeRGB, 1000 );
* {
  box-sizing: border-box; margin: 0; padding: 0;
}
html, body {
  height: 100%;
}
body {
  display: flex; flex-wrap: wrap;
  filter: saturate( 200% );
}
section {
  position: absolute;
  top: 0; right: 0; bottom: 0; left: 0;
  backdrop-filter: blur( 5rem );
  -webkit-backdrop-filter: blur( 5rem );
}
div {
  flex-grow: 1;
  min-width: 25%; min-height: 10%; background-color: #222;
  transition-property: background-color;
  transition-duration: 2s;
}
<section></section>

The function changeRGB():

function changeRGB() {
  const divs = body.querySelectorAll( `div` );
  
  for( let i = 0; i < divs.length; i++ ) {
    divs[ i ].style.backgroundColor = getRandomRGB();
    divs[ i ].style.transitionDuration = `${ getRandomNumber( 10 ) + 1 }s`;
  }
}

and the for loop after it:

for( let i = 0; i < 25; i++ ) {
  const div = document.createElement( `div` );
  
  div.style.backgroundColor = getRandomRGB();
  div.style.transitionDuration = `${ getRandomNumber( 10 ) + 1 }s`;
  body.appendChild( div ); 
}

contain almost identical lines of code:

divs[ i ].style.backgroundColor = getRandomRGB();
divs[ i ].style.transitionDuration = `${ getRandomNumber( 10 ) + 1 }s`;

and

div.style.backgroundColor = getRandomRGB();
div.style.transitionDuration = `${ getRandomNumber( 10 ) + 1 }s`;

respectively.

In changeRGB() we're manipulating div's that already exist and in the for loop we're first creating those div's then manipulating them in the same way. My goal is to refactor this code to make it a little less repetitive.

I need to replace div and divs[ i ] with something dynamic so I can just call one function and input the proper value here. I'm not sure if the loop complicates things more but I've tried window[ variable ].style.backgroundColor etc where variable is a dynamic variable but no luck. Possibly implementing it wrong.

How do I make changeRGB() function and for loop less repetitive and more concise?

function css html loops javascript
3 Answers

You could write another function like this:

function applyChange(element) {
    element.style.backgroundColor = getRandomRGB();
    element.style.transitionDuration = `${ getRandomNumber( 10 ) + 1 }s`;
}

You would just have to call it like so:

function changeRGB() {
  const divs = body.querySelectorAll( `div` );
  
  for( let i = 0; i < divs.length; i++ ) {
    applyChanges(divs[i]); // <- Call it here
  }
}

for( let i = 0; i < 25; i++ ) {
  const div = document.createElement( `div` );
  
  applyChanges(div); // <- Call it here
  body.appendChild( div ); 
}

This way you just pass the div element as a prompt and you can use it dynamically.

user_155265740

There are a few code-saving ways to approach this. For one, I find using ES6 arrow functions to allow for a more legible approach to multiple functions like this. To directly answer your question, the setting of backgroundColor and transitionDuration are identical in those 2 different functions, and can be outsourced, as I did below with applyTransform().

There are other space-savers, for example you can shorten something like this:

function getRandomRGB() {
  const colors = [];
  
  for( let i = 0; i < 3; i++ ) {
    const  n = getRandomNumber( 100 + 1 );   
    colors.push( n );
  }
  return `rgb(${ colors.join( '%,' )}%)`; 
}

into this:

const getRandomRGB = () => `rgb(${ [0,1,2].map(n => getRandomNumber( 100 + 1 )).join( '%,' )}%)`

Now, shortening code isn't always the best option as it can sometimes make it harder to read and maintain. However, I find this result to be easier to read than the original and at the same time it's 16 lines, vs the 32 from the original code.

const body = document.querySelector( `body` );
const getRandomNumber = max => Math.floor( Math.random() * max )
const getRandomRGB = () => `rgb(${ [0,1,2].map(n => getRandomNumber( 100 + 1 )).join( '%,' )}%)`
const changeRGB = () => divs.forEach(applyTransform); 
const applyTransform = div => {
  div.style.backgroundColor = getRandomRGB();
  div.style.transitionDuration = `${ getRandomNumber( 10 ) + 1 }s`;
}
for( let i = 0; i < 25; i++ ) {
  const div = document.createElement( `div` );
  applyTransform(div)
  body.appendChild( div ); 
}
const divs = body.querySelectorAll( `div` )
setInterval( changeRGB, 1000 );
* {
  box-sizing: border-box;
  margin: 0;
  padding: 0;
}

html,
body {
  height: 100%;
}

body {
  display: flex;
  flex-wrap: wrap;
  filter: saturate( 200%);
}

section {
  position: absolute;
  top: 0;
  right: 0;
  bottom: 0;
  left: 0;
  backdrop-filter: blur( 5rem);
}

div {
  flex-grow: 1;
  min-width: 25%;
  min-height: 10%;
  background-color: #222;
  transition-property: background-color;
  transition-duration: 2s;
  transition-timing-function: linear;
}
<section></section>

user_17729330

Well, the simplest solution is probably this:

//  div.style.backgroundColor = getRandomRGB();
//  div.style.transitionDuration = `${ getRandomNumber( 10 ) + 1 }s`;

… and this:

changeRGB();
setInterval( changeRGB, 1000 );

Remove the property generation at creation and run the function manually once if you don't want to wait a second.

user_19407100
Related