The click events runs only after clicking twice or three times

Question

    <html>
<head>
    <title>Random manga</title>
                    <script src="./js/my.js"></script>
    <link type="text/css" rel="stylesheet" href="./css/style.css">
</head>
<body>

<div id="al">

<div id="pic">
<img src="" id="img" style="visibility: hidden;">
</div>  
    <button id="btng" onclick="my()">Suggest me manga</button>
</div>

</body>
</html>

Here is the html for button div and other things Below is the css

    body
{
    background : white;
    color : black;
}

#pic
{
    border-radius : 25px;
    background-color : #f2f2f2;
    -webkit-border-radius : 35px;
    -moz-border-radius : 75px;
    box-shadow: rgba(0, 0, 0, 0.25) 0px 54px 55px, rgba(0, 0, 0, 0.12) 0px -12px 30px, rgba(0, 0, 0, 0.12) 0px 4px 6px, rgba(0, 0, 0, 0.17) 0px 12px 13px, rgba(0, 0, 0, 0.09) 0px -3px 5px;
    background-size : cover;
    
    text-align : center;
    height: 400px;
    width: 400px;
    background-size : cover;
    background-repeat: no-repeat;
    background-position: center; 
}


#btng

 {
  
  backface-visibility: hidden;
  background-color: #405cf5;
  border-radius: 6px;
  border-width: 0;
  box-shadow: rgba(50, 50, 93, .1) 0 0 0 1px inset,rgba(50, 50, 93, .1) 0 2px 5px 0,rgba(0, 0, 0, .07) 0 1px 1px 0;
  
  color: #fff;
  
  
  font-size: 100%;
  height: 44px;
  line-height: 1.15;
  margin: 12px 0 0;
  outline: none;
  overflow: hidden;
  padding: 0 25px;
  position: relative;
  text-align: center;
  text-transform: none;
  transform: translateZ(0);
  transition: all .2s,box-shadow .08s ease-in;
  

  width: 50%;
}



#btng:focus {
  box-shadow: rgba(50, 50, 93, .1) 0 0 0 1px inset, rgba(50, 50, 93, .2) 0 6px 15px 0, rgba(0, 0, 0, .1) 0 2px 2px 0, rgba(50, 151, 211, .3) 0 0 0 4px;
}
  


#al
{
    
    text-align : center;
    position : absolute;
    top : 50%;
    left : 50%;
    transform : translate(-50%, -50%);
}
#img{
    height: 100%;
    width: 100%;
}

And below the function for onclick on button

function my(){
    var bt=document.getElementById("btng");
    bt.textContent = "Suggest me another";
var my = new Array("im/R.jpg","im/S.jpg","im/E.jpg");
    var randomNum = Math.floor(Math.random() * my.length);
   var backgroundImage = my[randomNum];
     document.getElementById("img").src = my[randomNum];
document.getElementById("pic").style.backgroundImage=`url(${backgroundImage})`;


}

The onclick event only runs after the button is clicked twice or thrice sometimes I tried doing something to slove it but I was unsuccessful, is there any way to do it? Here is the live preview:- https://mangasuggestions.000webhostapp.com/index.html

Please click on buttons 4/5 times to get the problem I am facing.

When the array is that short, it's possible to consequentially get the same random number, and the background image isn't changed. — Teemu, Mar 28 '22 at 12:18
It certainly gets executed. Just add a `console.log("button clicked")` in your `my()` function and you will see it prints to the console. A way to prevent it could be storing the currently active image and then compare the generated value against the currently shown image and if they are the same re-generate the value. That could be done in a while loop. That's not a very efficient approach though as it is likely that you generate the same value a couple of times as the array is quite short. Nevertheless, It certainly won't really matter in terms of execution time. — Mushroomator, Mar 28 '22 at 12:23
So making a array big would slove the problem? @Mushroomator — Gaito Uchiha, Mar 28 '22 at 12:27
No making the array bigger won't prevent it, it will just make it less likely to happen as the chance of selecting the same image twice in a row = `1/n * 1/n` with `n` being the array length. — Mushroomator, Mar 28 '22 at 12:29
So what would be efficient way to get always unique value from array — Gaito Uchiha, Mar 28 '22 at 12:30
There are many ways, [here's one](https://stackoverflow.com/questions/2380019/generate-unique-random-numbers-between-1-and-100). — Teemu, Mar 28 '22 at 12:33
You could generate a random `step` value, meaning how far to move on from the current index within the range `1` till `array.length - 1` and then add that to the current index. Of course you would need to use modulo to make make sure you're not out of bounds. This would guarantee that the same picture will not be selected twice in a row. — Mushroomator, Mar 28 '22 at 12:33
Hmm ... "consequential" isn't a synonym for "sequential", which was what I meant in my first comment. — Teemu, Mar 28 '22 at 12:36

score 0 · Answer 1 · answered Mar 28 '22 at 12:19

0

Please make the button type="button". it seems working.

<button id="btng" type="button" onclick="my()">Suggest me another</button>

answered Mar 28 '22 at 12:19

Red-Dot

57
8

No it's not still working prefectly – Gaito Uchiha Mar 28 '22 at 12:21
Please check what you're getting in `console.log(randomNum);` when the image is not changing. might be getting same image index. then you'll have to change the concept of getting the random index from array. – Red-Dot Mar 28 '22 at 12:28

Andrew Li · Accepted Answer · 2022-03-28T12:51:45.660

0

The onclick event and the function is working properly. The image didn't update is because the randomNum is the same as the previous one.

Our goal is to prevent using the same number that is generated previously.

One way to do this is to keep track of the last generated random number, and regenerate until it is different from the previous one.

Main logic

    let randomNum;
    do {
        randomNum = Math.floor(Math.random() * images.length);
    } while (randomNum === prevRandomNum);
    prevRandomNum = randomNum

Full code (With some modification of your original code)

// Keep track of the last random number
let prevRandomNum = -1

function my(){
    const btn = document.getElementById("btng");
    btn.innerText = "Suggest me another";
    const images = ["im/R.jpg","im/S.jpg","im/E.jpg"];

    let randomNum;
    do {
        randomNum = Math.floor(Math.random() * images.length);
    } while (randomNum === prevRandomNum);
    prevRandomNum = randomNum

    const backgroundImage = images[randomNum];    
    document.getElementById("pic").style.backgroundImage=`url(${backgroundImage})`;
}

edited Mar 28 '22 at 12:51

answered Mar 28 '22 at 12:31

Andrew Li

117
1
8

I would add a check if there are atleast 2 images in the array to prevent a never ending while loop. – Mark Baijens Mar 28 '22 at 12:37
@MarkBaijens I am sorry, there are some incorrect logics. I have edited my answer and it should work now – Andrew Li Mar 28 '22 at 12:44
@AndrewLi You are not reassigning `pravRandomNum` anywhere. It is always `-1`. So `randomNum !== prevRandomNum` will be always `true` . – m51 Mar 28 '22 at 12:50
@m51 I am sorry, I forgot to change my code on the Main logic. The full code should be correct. – Andrew Li Mar 28 '22 at 12:51
Thanks your logic worked but can you tell me the logic in more detail – Gaito Uchiha Mar 28 '22 at 13:13
Why is - 1 is assigned as let value for randomNum? And why as a global variable? @AndrewLi – Gaito Uchiha Mar 28 '22 at 13:17
@GaitoUchiha -1 is used because it must be different from the random number that you generate in function `my()`. You can use any number you like. It is used as a global variable is because the value need to be store and stay as long as your web is running. If it is declare inside the function `my()`, then the value will be reinitialized after the execute the function. Not sure if it is a best practice, but it is fine in this simple web. – Andrew Li Mar 28 '22 at 13:33
You still have a potiontial never ending loop when there is only 1 image. – Mark Baijens Mar 28 '22 at 14:44
@MarkBaijens You are right! He should aware of this if he only get 1 image. But in this case, he don't even need the random number generator to generate image. – Andrew Li Mar 28 '22 at 15:52

m51 · Answer 3 · 2022-03-28T13:05:17.210

As @AndrewLi already answered, there is possibilty that generated random number is the same as the last one.

Instead of generating new number multiple times (with while loop).

You can filter out currently displayed image from array.

function my(){
    var bt=document.getElementById("btng");
    bt.textContent = "Suggest me another";
    var img = document.getElementById("img");
    var my = new Array("/im/R.jpg","/im/S.jpg","/im/E.jpg").filter((src) => src !== new URL(img.src).pathname);

    var randomNum = Math.floor(Math.random() * my.length);
    var backgroundImage = my[randomNum];
    document.getElementById("img").src = my[randomNum];
    document.getElementById("pic").style.backgroundImage=`url(${backgroundImage})`;
}

But to make it work, you need provide absolute path to your images in array.

So /im/R.jpg instead of im/R.jpg

Mushroomator · Answer 4 · 2022-03-28T13:38:51.257

Here is the probably most efficient way to do this without filtering the values causing a loop or re-generating values in case we get a duplicate using some index manipulation.

Explanation

For the first image we can select any image within the set of valid indices for the array i.e. 0 to array.length - 1 which we can do using Math.floor(Math.random() * array.length). Then we store the selected index in a variable currentImageIdx.

For any following image we generate a step value which determines the amount of steps we want to move on from the current index to the next value. We need to choose this in the range of 1 to array.length - 1 which we can do by using Math.floor(1 + Math.random() * (arrayLength - 1)) as this will make sure we will never move on so far that we are back at the same index as we currently are. We then just need to add that step value to the current index and take the remainder using modulo to make sure we're not getting out of bounds.

Examples for moving on to next index

For the following examples an array with 3 values is assumed:

We're currently at index 0 so we should either move on 1 or 2 steps. If we were to move on 3 steps we would be at 0 again because (start index + step) % array.length would translate to (0 + 3) % 3 = 0 and we'd be at the start index again.
If we're at index 1 we could again move on 1 or 2 steps but not 3 because (start index + step) % array.length would translate to (1 + 3) % 3 = 1 and we'd be at the start index again.
The same applies for index 2

This will work for an array of any size. Except that for the case of just having one element in the array that one element will of course be selected every time.

let currentImageIdx = undefined;

function my() {
  var bt = document.getElementById("btng");
  bt.textContent = "Suggest me another";

  var my = new Array("https://dummyimage.com/20x20/000/fff", "https://dummyimage.com/20x20/00FF00/000", "https://dummyimage.com/20x20/FF0000/000");
  let backgroundImageIdx;
  // it is imporant to check for undefined here as currentImageIdx != 0 would also trigger this case if it just was if(!currentImageIdx) and may produce the same picture twice 
  if (currentImageIdx !== undefined) backgroundImageIdx = getNextImage(currentImageIdx, my.length)
  else backgroundImageIdx = Math.floor(Math.random() * my.length);
  document.getElementById("img").src = my[backgroundImageIdx];
  currentImageIdx = backgroundImageIdx;
  console.log(`selected image ${currentImageIdx}`);
  document.getElementById(
    "pic"
  ).style.backgroundImage = `url(${my[backgroundImageIdx]})`;
}

function getNextImage(currentIndex, arrayLength) {
  // generate random step value between 1 and array.length - 1
  // with three elements: either 1 and 2
  var step = Math.floor(1 + Math.random() * (arrayLength - 1));
  // add the step value to current index and make sure we're not out of bounds
  console.log(`(${currentIndex} + ${step}) % ${arrayLength} = ${(currentIndex + step) % arrayLength}`)
  return (currentIndex + step) % arrayLength;
}

<div id="al">

  <div id="pic">
    <img src="" id="img" style="visibility: hidden;">
  </div>
  <button id="btng" onclick="my()">Suggest me manga</button>
</div>

The click events runs only after clicking twice or three times

4 Answers4

Main logic

Full code (With some modification of your original code)

Explanation

Examples for moving on to next index