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

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 Mar 27 at 19:52
    
Might be useful to use this instead of parent, since this'll refer to the game1 element automatically. – Flambino Mar 28 at 4:18
    
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 Mar 28 at 4:20
    
@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 Mar 28 at 4:31
    
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 Mar 28 at 4:36

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.