0

I have this code that remove Items in class btn-danger. I just want to know what can I use instead of for loop to make the code cleaner:

var removeCartItemButtons = document.getElementsByClassName('btn-danger');
for (var i = 0; i < removeCartItemButtons.length; i++) {
 var button = removeCartItemButtons[i];
 button.addEventListener('click', function (e) {
  var buttonClicked = e.target;
  buttonClicked.parentElement.parentElement.remove();
 });
}
Tom Yotwongjai
  • 47
  • 8
  • 1
    Take advantage of event bubbling and instead of adding a separate even listener on each `btn-danger` element, add a single click listener on the common parent element of all `btn-danger` elements. – Yousaf Dec 31 '21 at 12:04
  • In general, use [`Array` methods](//developer.mozilla.org/docs/Web/JavaScript/Reference/Global_Objects/Array#Static_methods). However, in the case of events, as the previous comment says, use [event delegation](//developer.mozilla.org/docs/Learn/JavaScript/Building_blocks/Events#Event_delegation) instead of assigning multiple event listeners — it’s more maintainable, and applies to dynamically added elements. Use an [event argument](//developer.mozilla.org/docs/Web/API/EventTarget/addEventListener#The_event_listener_callback)’s [`target`](//developer.mozilla.org/docs/Web/API/Event/target). – Sebastian Simon Dec 31 '21 at 12:06
  • See [the tag info](/tags/event-delegation/info), [What is DOM Event delegation?](/q/1687296/4642212), and [Want to add "addEventListener" on multiple elements with same class](/q/51573435/4642212). – Sebastian Simon Dec 31 '21 at 12:07
  • `Array.from(document.getElementsByClassName('btn-danger')).forEach((button) => button.addEventListener('click', (e) => e.target.parentElement.parentElement.remove()));` – 0stone0 Dec 31 '21 at 12:07
  • What I'm most worried about is the `.parentElement.parentElement` part. this is incredibly prone to errors. as soon as you put as little as a `` inside one of the buttons, or have to add a `
    ` for layout purposes this will break. check out [Element#closest()](https://developer.mozilla.org/en-US/docs/Web/API/Element/closest)
     – Thomas Dec 31 '21 at 12:35

2 Answers2

1

General Improvement

  • Use let instead of var. See This
  • If you're just accessing a variable and not changing it, use const instead of let.
  • document.querySelectorAll() or document.querySelector() are in some cases (like in your case) better than getElementsByClassName or getElementById or getElementsByTagName.
  • When iterating over arrays or static node lists, forEach is generally a better (more readable) option than a for loop. (querySelectorAll returns a static node list. You don't need Array.from, but if you want to use other Array specific methods use Array.from as @Yousaf pointed out)
  • Use functionin the global scope and for Object.prototype properties. Use class for object constructors. Use => everywhere else. See This.

Here is how I would have written it

document.querySelectorAll('.btn-danger').forEach(btn=>btn.addEventListener('click',e=>e.target.parentElement.parentElement.remove()))
Never Serious
  • 91
  • 4
0

Your code can be shortened to:

Array.from(document.getElementsByClassName('btn-danger')).forEach((button) => {
    button.addEventListener('click', (e) => {
        e.target.parentElement.parentElement.remove();
    });
});

This uses:

Since the array functions can be re-written without the {}, an one-liner would look like:

Array.from(document.getElementsByClassName('btn-danger')).forEach((button) => button.addEventListener('click', (e) => e.target.parentElement.parentElement.remove()));
0stone0
  • 34,288
  • 4
  • 39
  • 64
  • Just out of curiosity; is there any particular reason/advantage for preferring `Array.from(...).forEach(...)` over `document.querySelectorAll('.btn-danger').forEach(...)`, please? Thanks – secan Dec 31 '21 at 12:18
  • Yes @secan, that won't work since nodelists don't have a foreach. Please see [this answer](https://stackoverflow.com/a/39146365/5625547). – 0stone0 Dec 31 '21 at 12:20
  • [`document.querySelectorAll()`](https://developer.mozilla.org/en-US/docs/Web/API/Document/querySelectorAll#accessing_the_matches) returns a static node list that can be iterated with `forEach()`, as you can see here: https://jsfiddle.net/ujkwvath/ – secan Dec 31 '21 at 12:27
  • Ow sorry thought you were using getElementsByClassName. Not sure if queryAll is better than array.from. – 0stone0 Dec 31 '21 at 12:29