Home > Software design >  How to refactor my code so it's not repeating the same thing
How to refactor my code so it's not repeating the same thing

Time:12-06

I have 3 different buttons on my page, and when you click on one it checks the corresponding radio button.

Currently, I have each button with its own onclick function:

onclick="radioChecked1()"
onclick="radioChecked2()"
onclick="radioChecked2()"

And then there are the functions:

function radioChecked1() {
  var package1 = document.querySelector("#package1");
  package1.setAttribute("checked", 1);
}
function radioChecked2() {
  var package2 = document.querySelector("#package2");
  package2.setAttribute("checked", 1);
}
function radioChecked3() {
  var package3 = document.querySelector("#package3");
  package3.setAttribute("checked", 1);
}

These functions are doing the same thing, the only thing that changes is the number in the id of the input it's selecting.
I'm sure there's a way to simplify this into one function instead of a separate one for each button, I don't know how to do it.

CodePudding user response:

You can refactor this code by simplifying the function and using parameters:

function radioChecked(id) {
    var package = document.querySelector(id);
    package.setAttribute("checked", 1);
}

Then on your buttons call the function with the corresponding id:

onclick="radioChecked('#package1')" onclick="radioChecked('#package2')" onclick="radioChecked('#package3')"

CodePudding user response:

It depends a little on how your markup is written but if you add data attributes to both buttons and the radio buttons you can take the id from a button when it's clicked, and then find the corresponding id on a radio input.

Here I've wrapped the buttons and radio input in their own containers so that I can use event delegation - adding one listener to a parent container that catches events from its child elements as they bubble up the DOM, rather than attaching listeners to all of the elements.

// Instead of inline JS we cache the containers elements
// and then add one listener to the buttons container
const radios = document.querySelector('.radios');
const btns = document.querySelector('.btns');
btns.addEventListener('click', handleClick);

// If the clicked element is a button
// extract its id from its dataset, look for
// the corresponding radio input, and update the
// `checked` property
function handleClick(e) {
  if (e.target.matches('button')) {
    const { id } = e.target.dataset;
    const radio = radios.querySelector(`[data-id="${id}"]`);
    radio.checked = true;
  }
}
fieldset { margin-top: 1em; }
<fieldset >
  <legend>Buttons</legend>
  <button data-id="radio1" type="button">Button 1</button>
  <button data-id="radio2" type="button">Button 2</button>
  <button data-id="radio3" type="button">Button 3</button>
</fieldset>

<fieldset >
  <legend>Radio buttons</legend>
  <label for="radio1">Radio1
    <input data-id="radio1" type="radio" name="radioset">
  </label>
  <label for="radio2">Radio2
    <input data-id="radio2" type="radio" name="radioset">
  </label>
  <label for="radio3">Radio3
    <input data-id="radio3" type="radio" name="radioset">
  </label>
</fieldset>

Additional documentation

CodePudding user response:

Radio input have values...

const myForm = document.querySelector('#my-form')
  
myForm.onsubmit = e => e.preventDefault(); // disable form submit as testing it

myForm.onclick = e =>  // event delegation
  {
  if (!e.target.matches('button[data-val]')) return  // exit...
  
  myForm.radioset.value = e.target.dataset.val;
  //     ^-- use the name 
  }
label { margin-right : 2em; }
<form id="my-form">
  <fieldset>
    <legend>Buttons</legend>
    <button data-val="radio1" type="button">Button 1</button>
    <button data-val="radio2" type="button">Button 2</button>
    <button data-val="radio3" type="button">Button 3</button>
  </fieldset>

  <fieldset>
    <legend>Radio buttons</legend>
    <label>
      <input value="radio1" type="radio" name="radioset">
      Radio1
    </label>
    <label>
      <input value="radio2" type="radio" name="radioset">
      Radio2
    </label>
    <label>
      <input value="radio3" type="radio" name="radioset">
      Radio3
    </label>
  </fieldset>
</form>

  • Related