Browser freezes when generating random background color for body element!

JavaScript
1

I am trying to make my background color different each time when the button is clicked and I don’t want to get the same color in a row so I did a check, if the current value is the same as previous value, then get the value again and if current value is different to previous value then return the value, but there is a problem, when I first implement the check, using the recursive way, everything works fine

(Example code using recursive way)

let previousBackgroundColor = "";

const getRandomBackgroundColor = (backgroundColorsArray) => {
  const getRandomNum = Math.floor(Math.random() * backgroundColorsArray.length);
  let randomBackgroundColor = backgroundColorsArray[getRandomNum];
  // Prevent show same background color in a row
  if (previousBackgroundColor === randomBackgroundColor) {
    return getRandomBackgroundColor(backgroundColors);
  } else {
    previousBackgroundColor = randomBackgroundColor;
    return randomBackgroundColor;
  }
};

but, when I try to implement the check using other than recursive way(using do…while & while loop), the browser freezes when I keep changing the background color of the body by clicking the button fast, it doesn’t have this kind of problem when using recursive way

(Example code using do…while way)

let previousBackgroundColor = "";

const getRandomBackgroundColor = (backgroundColorsArray) => {
  const getRandomNum = Math.floor(Math.random() * backgroundColorsArray.length);
  let randomBackgroundColor = '';
  // Prevent show same background color in a row
  do{
    randomBackgroundColor = backgroundColorsArray[getRandomNum];
  } while(previousBackgroundColor === randomBackgroundColor)

  previousBackgroundColor = randomBackgroundColor;
  return randomBackgroundColor;
};

What am I doing wrong?

Other than recursive way to solve the problem, is using do…while or while loop the right approach?

Is recursive the only way to solve this problem?

(Full code that has the problem)

const bodyEl = document.querySelector("body");

const buttonEl = document.querySelector("button");

const backgroundColors = [
  "#3D315B",
  "#444B6E",
  "#9EB7E5",
  "#EF959D",
  "#CBE896",
  "#596F62",
  "#F29559",
  "#B185A7",
  "#BFD1E5",
  "#B7C0EE",
  "#7067CF",
  "#CBF3D2",
];

let previousBackgroundColor = "";

const getRandomBackgroundColor = (backgroundColorsArray) => {
  const getRandomNum = Math.floor(Math.random() * backgroundColorsArray.length);
  let randomBackgroundColor = "";
  do {
    randomBackgroundColor = backgroundColorsArray[getRandomNum];
  } while (previousBackgroundColor === randomBackgroundColor);

  previousBackgroundColor = randomBackgroundColor;
  return randomBackgroundColor;
};

const changeBackgroundColor = () => {
  bodyEl.style.backgroundColor = getRandomBackgroundColor(backgroundColors);
};

buttonEl.addEventListener("click", changeBackgroundColor);
2

In both solutions, your random color is gotten at the the top of the function. It is a fixed value for the entire run of the function. This works for the recursive solution because it gets a new function call on each iteration - therefore it gets a new random value on each call. But for the iterative solution, there is only one function call so there is only one random color, so it if happens to match the previous color, that loop is going to run forever because the random one never changes.

To get it to work, you need to create a new random color (or the index) inside your loop, either by placing the code there or by putting the code in a helper function that is called inside the loop.

1 Like
3

So for using the recursive solution, how do I make the value not fixed or the way how I implement is how people normally do ? and with iterative solution, I still don’t quite get what you mean by “To get it to work, you need to create a new random color (or the index) inside your loop”, I thought

randomBackgroundColor = backgroundColorsArray[getRandomNum];

the code will re-run and get a new background color value when previous background color value and current background color value match…

4

And just a few points on code…

const getRandomBackgroundColor = (backgroundColorsArray) => {

Here, you have the array being passed in and the previous color as global. That is the reverse of what I expect. I would expect the array to be a constant since it is never changing. The previous color is changing so it is a dependency of the function - I would expect that to be passed in. I would just pass in the current color.

Also, the naming of _ getRandomNum_ is a bit odd to me - I would expect something with an action verb as the first word to be a function. That is a pretty common naming convention.

5

but where are you changing the random number?

1 Like
6

So for using the recursive solution , how do I make the value not fixed or the way how I implement is how people normally do ?

I don’t understand. That solution works fine as it is. It is fixed, and it’s what you want.

I still don’t quite get what you mean by “To get it to work, you need to create a new random color (or the index) inside your loop”, I thought

randomBackgroundColor = backgroundColorsArray[getRandomNum];

the code will re-run

Well, true, it will rerun. But since getRandomNum is not changing in each iteration of the loop, the value of randomBackgroundColor will never change. And if it happens to be the same as the previous color, that loop will run forever.

So, you need that getRandomNum to change on each run so that randomBackgroundColor will change. One way would be to move

const getRandomNum = Math.floor(Math.random() * backgroundColorsArray.length);

inside the loop so it gets rerun every iteration. The other option would be to turn getRandomNum into a function and call it as:

    randomBackgroundColor = backgroundColorsArray[getRandomNum()];

Personally, I’m not a fan of using recursion for the sake a recursion - I think the iterative solution is better. There is actually an O(1) solution - I’ll see if I can find a post that talks about that.

1 Like
7

Here is a discussion I had about a solution that does not involve multiple passes to ensure that you get a new value. It is for random quotes, but the idea is the same.

8

This is my final code

const generateRandomNum = (array) => {
  const randomNum = Math.floor(Math.random() * array.length);
  return randomNum;
};

let previousBackgroundColorValue = "";

const getRandomBackgroundColor = () => {
  const randomIndex = () => generateRandomNum(backgroundColors);
  let backgroundColorValue = '';

  do{
    backgroundColorValue = backgroundColors[randomIndex()];
  }while(backgroundColorValue === previousBackgroundColorValue);
  previousBackgroundColorValue = backgroundColorValue;
  return backgroundColorValue;
};

Is this correct? Is there anything I should change or clean up so the code looks more prettier?

I still feel above codes are kind weird, if it was you how would you write the codes?
also
if you are using recursive solution, how would you write your code?

9

That is basically what I would right if I were doing an iterative solution. A few differences are that again, I would pass in the previous colors, and now it would make sense to call it getRandomNum or getRandomIndex since it is a function. But other than that, just looking it over, it looks sound. And I don’t think you need to store the previous color - just pass in the old color to the function - it only needs to live there.

As mentioned, I wouldn’t use a recursive solution for this, unless just as an exercise. Recursive solutions can be arcane and in certain cases can be memory hogs as they push onto the call stack. Granted, that is highly unlikely in this case, but I think too much emphasis is placed on recursion. There is always an iterative solution, which is often simpler. Granted, there are a few types of problems where recursion is the perfect, easiest, cleanest solution. In 2.5 years as a developer, I’ve only used recursion once - when I had to search a large tree. I think coders obsess about recursion because it is “sexy”. But it’s good to know because it sometimes shows up on interview questions and on rare occasions, it can be a life saver.

But I wouldn’t use the iterative solution either. I would use the one-pass, O(1), solution, as explained in the link provided above.

But still, you’ve come up with a couple of good solutions - be proud of that.

1 Like
10

Oh, I did notice one thing:

const randomIndex = () => generateRandomNum(backgroundColors);

Seems redundant to me. I would eliminate this can just call it inside the loop. And rather than pass in the array and get the length inside generateRandomNum, I would pass in the length, making generateRandomNum more generic, just accepting the max value as a parameter.

There are some other little things that I think are a bit redundant.

But those are minor things - the code itself is sound.

11

I don’t have time to test it, but if I did an iterative solution, it would look something like:

const generateRandomIndex = size => Math.floor(Math.random() * size);

const getRandomBackgroundColor = previousBackgroundColorValue => {
  let backgroundColorValue;

  do {
    backgroundColorValue = backgroundColors[generateRandomIndex(backgroundColors.length)];
  } while (backgroundColorValue === previousBackgroundColorValue);
  
  return backgroundColorValue;
};