current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
up vote 2 down vote favorite
1

In my program below I am getting the parent node game 1 and then removing one of the two child nodes, depending on which one is clicked. I currently have two event listeners, with two separate functions for removing one child vs the other. It doesn't feel very dry to me, and I think I could benefit from having someone skilled in JS take a look at this and show me how this could be more easily accomplished.

var firstWinner = document.getElementById('game1');

function removefirst() {
    firstWinner.removeChild(firstWinner.childNodes[0]);
}

function removeSecond() {
    firstWinner.removeChild(firstWinner.childNodes[1]);
}

var gameOneNodes = document.getElementById('game1').childNodes;
gameOneNodes[1].addEventListener('click', removefirst, false)
gameOneNodes[0].addEventListener('click', removeSecond, false)
share|improve this question
up vote 5 down vote accepted

You should define one click handler that removes all siblings of the clicked element. That is, remove all children of the parent, except the target of the event itself.

document.getElementById('game1').addEventListener('click', function(event) {
    // Remove all siblings of the clicked element
    for (var c = this.childNodes.length - 1; c >= 0; c--) {
        if (this.childNodes[c] != event.target) {
            this.removeChild(this.childNodes[c]);
        }
    }
});
<div id="game1">
  <button>Alpha</button>
  <button>Bravo</button>
  <button>Charlie</button>
  <button>Delta</button>
</div>

You could also iterate this way, but there's an awkward c--:

for (var c = 0; c < this.childNodes.length; c++) {
    if (this.childNodes[c] != event.target) {
        this.removeChild(this.childNodes[c--]);
    }
}
share|improve this answer
    
Thank you for taking the time. This is really great. – Jbur43 yesterday
    
Might be useful to use this instead of parent, since this'll refer to the game1 element automatically. – Flambino yesterday
    
Also, while a little arcane, the for-loop can be replaced with [].slice.call(this.childNodes).forEach(...). Not that it'd be inherently better or anything, it's just an alternative. – Flambino yesterday
    
@Flambino I've incorporated your suggestion to use this instead of event.target.parentNode. Your slice() suggestion is too tricky for my taste, though. – 200_success♦ yesterday
    
That's fair. I'm not a fan of the slice trick either, per se, but it's the simplest way to get a proper array from annoying JS/DOM array-like objects (like NodeList here, or the arguments obj). Again, not necessary in this case, but I figured I put it in a comment to mention it (no need to put it in the answer) – Flambino yesterday

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.