CODEWITHSUNDEEP
javascripthtml
sagar
sagar

Reputation: 29

How to add on-click event to button in javascript?

<html>
<body>
<input type="text" id="number"/>
<input type="button" value="create button" onclick="createbtn()"/> 
<br> 
<script>
function createbtn() 
{
  var n=document.getElementById("number").value;

  for(i=1;i<=n;i++)
   {
    var x = document.createElement("INPUT");
    x.setAttribute("type", "button");
    x.setAttribute("value", i);
    x.setAttribute("id","x"+i);
    document.body.appendChild(x);
    document.getElementById("x"+i).onclick=function(){rotate(i)};            
   }
}

function rotate(p)
  { 
   var n=document.getElementById("number").value;
   var j=n;
   var k=0; 

   for(i=n;i>p;i--)
    {
     document.getElementById("x"+i).value=i-p;
    }
   for(i=1;i<=p;i++)
    {
     document.getElementById("x"+i).value=(j-(p-1))+k; 
     k++;
    }
  }

</script>
</body>
</html>

In the above code what I am trying to do is create number of buttons equal to number inputted in text field and then add an on-click event to those buttons so that when they are clicked the value of each button is rotated. The method I have used for getting an onclick event is not working properly. Help me!!

Upvotes: 2

Views: 2411

Answers (1)

Mitya
Mitya

Reputation: 34556

Several issues here.

1) Use meaningful variable names. x and p are hardly indicative and will make life difficult when you come back to the code one day and wonder what they do. (They also hinder people trying to help you on Stack Overflow, who have to first interpret them.)

2) You don't need to do document.getElementById("x"+i).onclick - you already have a reference to the button in x so you can just do x.onclick.

3) As a rule, don't use DOM-0 events (e.g. .onclick). These allow you only one of each type and are less manageable. Instead, use .addEventListener().

4) As per @Alex Kudryashev's comment, you are making the common beginner mistake of trying to reference an iterative variable inside an asynchronous closure. In other words, by the time rotate() runs, p will always evaluate to the last value of i, because it happens after the loop has finished. Instead, you need to pass a run-time copy of i in at the point of binding the event handler. This can be done with the help of an immediately-executed function which returns the event handler. (This can be a little daunting if you're new to JS and is thus outside the scope of this answer. Look it up, though.)

5) Rather than binding separately to each and every button, look into event delegation. This makes your code more efficient and can help performance when you're dealing with an appreciably high number of elements and events.

All in all, try this:

x.addEventListener('click', (function(i_local_copy) { return function() {
    rotate(i_local_copy);
}; })(i));

Upvotes: 5

Related Questions