Is there anyway to simplify this Javascript code

Question

I have this code that, when a certain card is clicked, its content is displayed on an overlay card. But the way I have it right now is to repetitive:

HTML:

        <div class="card c1">
            <img src="max.png" width="65px">
            <div class="text">
                <h3 class="firstName">Owen</h3>
                <h3 class="lastName">Osagiede</h3> 
                <p>[email]</p> 
                <p>[city]</p>
            </div>
        </div>
 
        <div class="card c2">
            <img src="max.png" width="60px">
            <div class="text">
                <h3 class="firstName">Kanye</h3>
                <h3 class="lastName">West</h3> 
                <p>[email]</p> 
                <p>[city]</p>
            </div>
        </div>

        <div class="card c3">
            <img src="max.png" width="65px">
            <div class="text">
                <h3 class="firstName">Quando</h3>
                <h3 class="lastName">Rondo</h3> 
                <p>[email]</p> 
                <p>[city]</p>
            </div>
        </div>

JS:

      function overlayUser(){
         card[1].addEventListener('click', function(){
           first.innerHTML = card[1].getElementsByTagName('h3')[0].innerHTML;
           last.innerHTML = card[1].getElementsByTagName('h3')[1].innerHTML;

    });
    card[2].addEventListener('click', function(){
        first.innerHTML = card[2].getElementsByTagName('h3')[0].innerHTML;
        last.innerHTML = card[2].getElementsByTagName('h3')[1].innerHTML;
    });
    card[3].addEventListener('click', function(){
        first.innerHTML = card[3].getElementsByTagName('h3')[0].innerHTML;
        last.innerHTML = card[3].getElementsByTagName('h3')[1].innerHTML;
    });

I have tried to loop over it with a for loop, but keep getting an error:

      `function overlayUser(){
          for (i = 0; i < card.length; i++){
              card[i].addEventListener('click', function(){
                first.innerHTML = card[i].getElementsByTagName('h3')[0].innerHTML;
               last.innerHTML = card[i].getElementsByTagName('h3')[1].innerHTML;
               });
           }
        }`

In your first snippet, you start at `card[1]`. But your loop is starting at `i=0`. Are you intending to add a listener to `card[0]` too? — JoshG, Sep 06 '20 at 00:39
The answer you've selected as "the" answer has some very poor performing and potentially insecure code in it. See my answer for the better solution. — Scott Marcus, Sep 06 '20 at 01:04
both answers are correct and the one you **selected** is good as well. Just use `textConent` when getting the `h3` text instead of using `innerHTML` - thats all — Always Helping, Sep 06 '20 at 01:07
@AlwaysHelping The answer by slebetman is most certainly not good. It's very inefficient and opens up a security hole. — Scott Marcus, Sep 06 '20 at 01:08

slebetman · Accepted Answer · 2020-09-06T00:54:52.943

In a DOM event handler, the current element is this. Therefore you can write a single function for all of them:

function handleClick () {
    first.innerHTML = this.getElementsByTagName('h3')[0].innerHTML;
    last.innerHTML = this.getElementsByTagName('h3')[1].innerHTML;
}

function overlayUser(){
    for (i = 0; i < card.length; i++){
        card[i].addEventListener('click', handleClick);
    }
}

The this API is the original API for finding out which element caused the event. Thus it is very compatible with all browsers.

Alternatively, if you feel uncomfortable mixing the usage of this you can also find out the current element from the event object:

function handleClick (event) {
    let card = event.target;

    first.innerHTML = card.getElementsByTagName('h3')[0].innerHTML;
    last.innerHTML = card.getElementsByTagName('h3')[1].innerHTML;
}

The event object is a slightly less ancient API but is compatible with everything from IE8 and above.

Additionaly you can use event bubbling/capturing to even get rid of the for loop. Just install the event on the parent element of all three cards and let event.target sort out which card caused the event:

parentDiv.addEventListener('click', handleClick);

`getElementsByTagName('h3')[0].innerHTML` <-- There is so much wrong with this. — Scott Marcus, Sep 06 '20 at 00:59

Scott Marcus · Answer 2 · 2020-09-06T16:11:05.263

Instead of looping over all the individual elements that you want to have event handlers and hooking each up, set a single handler on an ancestor element and allow the event to bubble up to that element. Then, when handling it, look at the event.target, which refers to the actual element that triggered the event. This is called event delegation.

Also, do not use .getElementsByTagName() in 2020. That is a 25+ year old API that returns a live node list that can dramatically hurt performance, especially since you are only interested in a single element when you use it.

Addtionally, never use .innerHTML if you can avoid it. It has security and performance implications. Since you aren't actually working with a string that needs any HTML parsed, you should use .textContent.

Finally, you should not be using h3 unless it is to create a sub-section of a pre-existing h2. Headings are meant to divide your document into ordered sections and these sections are used by those who rely on assistive technologies to navigate a document. If you are just using the h3 because of the styling the browser applies to the text, you should instead just use a p and then use CSS to style it the way you want.

// Get references to first and last (for this demo)
let first = document.querySelector(".first");
let last = document.querySelector(".last");

// Just handle the click event at the wrapper of all the cards
document.querySelector(".wrapper").addEventListener("click", function (event){
   // Then access the content of the card that actaully triggered the event
   first.textContent = event.target.closest(".card").querySelector("h3").textContent;
   last.textContent = event.target.closest(".card").querySelector("h3:nth-child(2)").textContent;
});

/* Just for demo */
.results {
  position:sticky;
  left:50%;
  top:0;
  background-color:#e0e0e0;
  border:2px solid red;
}

<div class="results">
  <div class="first"></div>
  <div class="last"></div>
</div>

<div class="wrapper">
  <div class="card c1">
            <img src="max.png" width="65px">
            <div class="text">
                <h3 class="firstName">Owen</h3>
                <h3 class="lastName">Osagiede</h3> 
                <p>[email]</p> 
                <p>[city]</p>
            </div>
        </div>
 
        <div class="card c2">
            <img src="max.png" width="60px">
            <div class="text">
                <h3 class="firstName">Kanye</h3>
                <h3 class="lastName">West</h3> 
                <p>[email]</p> 
                <p>[city]</p>
            </div>
        </div>

        <div class="card c3">
            <img src="max.png" width="65px">
            <div class="text">
                <h3 class="firstName">Quando</h3>
                <h3 class="lastName">Rondo</h3> 
                <p>[email]</p> 
                <p>[city]</p>
            </div>
        </div>
</div>

Is there anyway to simplify this Javascript code

2 Answers2