HTML Button calls a wrong function onclick

Question

I made a html document that is basically a program that converts a decimal number into binary. It works. I added a 'Reverse' button that would do the opposite (convert binary into decimal) and for some reason it runs the code that should be run by the 'Convert' button. This is the code:

function decimalToBinary() {
  document.getElementsByClassName('inputBox')[0].placeholder = 'Enter decimal number'
  alert('Hello')
}

function binaryToDecimal() {
  document.getElementsByClassName('inputBox')[0].placeholder = 'Enter binary number'
  alert('Hi')
}

function reverse() {
                if (document.getElementById('decimalNumber').placeholder === 'Enter decimal number') {
                    document.getElementById('convertButton').onclick = binaryToDecimal()
                } else if (document.getElementById('decimalNumber').placeholder === 'Enter binary number') {
                    document.getElementById('convertButton').onclick = decimalToBinary()
                }
            }

<input type="number" class="inputBox" id="decimalNumber" placeholder="Enter decimal number" min="0" step="0.0001">
<button id="convertButton" class="button hoverTurquoise" onclick="decimalToBinary()">Convert</button>
<button class="button hoverTurquoise reverse" onclick="reverse()">Reverse</button>

(I replaced all the code that converts the numbers into alert messages so it's easier to understand)

If anyone's got any ideas on how to fix this, please let me know. Thanks

You're **calling** `binaryToDecimal` and settings its return value as the `onclick`. This is exactly like the linked duplicate, you're just assigning to `onclick` instead of passing a function to `setTimeout`. See the answers there. :-) — T.J. Crowder, Oct 23 '18 at 13:53

Scott Marcus · Accepted Answer · 2018-10-23T19:22:01.427

In JavaScript, functions are data -- you can refer to them as data. In your code, when you should be referring to them this way, you are adding () to the end of the function name, thus invoking the function, rather than referring to it.

So, your functions are actually being executed right away (before anyone clicks any buttons) and the return value from them (which is nothing) is what is being assigned as the value of the onclick property. So, your "reverse" button does call the reverse() function when you click it, but the onclick assignments you are doing in that function are not actually setting the onclick property to anything.

Your code should be:

if (document.getElementById('decimalNumber').placeholder === 'Enter decimal number') {
   document.getElementById('convertButton').onclick = binaryToDecimal;
} else if (document.getElementById('decimalNumber').placeholder === 'Enter binary number') {
   document.getElementById('convertButton').onclick = decimalToBinary;
}

Now, while setting up event handlers using inline HTML event attributes, such as onclick and setting up event handlers as JavaScript object properties can work, they are both very old techniques that have big drawbacks. Read this to see why you should really never use inline HTML event attributes. And, as far as event properties, their drawback is that you can't assign multiple event handlers to the same object event.

Instead of either of these legacy techniques, use the modern, standards-based approach of .addEventListener().

One last thing, avoid scanning the document repeatedly for the same element over and over, that's just a waste of resources.

Here's your code re-worked to use this and you'll see how much cleaner it is.

// Get references to the elements you'll need to use more than once:
let btnConvert = document.getElementById('convertButton');
let btnReverse = document.querySelector("button.reverse");
let input = document.getElementById("decimalNumber");

// Now, set up event handlers for the buttons the modern way 
// (notice there are no () after the function names because we
// don't want to invoke them right now, we only want to refer to them.)
btnConvert.addEventListener("click", decimalToBinary);
btnReverse.addEventListener("click", reverse);

function decimalToBinary() {
  input.placeholder = 'Enter decimal number';
}

function binaryToDecimal() {
  input.placeholder = 'Enter binary number';
}

function reverse() {
  if (input.placeholder === 'Enter decimal number') {
    // We can also remove event handlers in much the same way as when we add them:
    btnConvert.removeEventListener("click", decimalToBinary);
    btnConvert.addEventListener("click", binaryToDecimal);
  } else {
    btnConvert.removeEventListener("click", binaryToDecimal);
    btnConvert.addEventListener("click", decimalToBinary);
  }
}

<input type="number" class="inputBox" id="decimalNumber" placeholder="Enter decimal number" min="0" step="0.0001">

<!-- Don't set up event handlers in HTML -->
<button id="convertButton" class="button hoverTurquoise">Convert</button>
<button class="button hoverTurquoise reverse">Reverse</button>

Thank you so much. This really helped :D – Lovro Ledić Oct 23 '18 at 19:19 — Lovro Ledić, Oct 23 '18 at 19:19

score 0 · Answer 2 · answered Oct 23 '18 at 13:52

You have to return the function itself, not its return value, so you have to remove the parantheses.

if (document.getElementById('decimalNumber').placeholder === 'Enter decimal number') {
  document.getElementById('convertButton').onclick = binaryToDecimal
} else if (document.getElementById('decimalNumber').placeholder === 'Enter binary number') {
  document.getElementById('convertButton').onclick = decimalToBinary
}

HTML Button calls a wrong function onclick

2 Answers2