Basic ul li elements in JavaScript listeners

Question

I am learning JavaScript and I have written a code snippet. Can someone suggest some improvements?

Array.prototype.pushIfDoesntExist = function (item){
 if(this.indexOf(item) === -1) {
 this.push(item);
 return true;
 }
 return false;
}
var Testing = {
		clickedList: [],
		applyListeners: function() {
 			var self = this;
 var nodes = document.getElementsByTagName('li');
 		for (var i = 0; i < nodes.length; i++) {	
 		document.getElementsByTagName('li')[i].addEventListener('click', function() {
 	//we only want to display an item once
 	self.clickedList.pushIfDoesntExist(this.textContent)
 self.displayData();
 		});
 }
 },
 displayData: function() {
 		var textBox = document.getElementById('clickedElements');
 textBox.textContent = this.clickedList.join(', ');
 }
}
Testing.applyListeners();

<ul>
 <li>Milk</li>
 <li>Eggs</li>
 <li>Bacon</li>
 <li>Cheese</li>
</ul>
<div id="clickedElements">
</div>

• Phrancis: I think you should extract the logic if(this.indexOf(item) === -1) into its own function like if(itemDoesNotExist(this, item)) to make the code more clear.

Answer 1

Don't Modify Array.prototype

Writing custom functions to built in prototypes is generally a bad idea. The basic reasoning behind this is that it is a bad practice to modify something you don't own, but check out this Stack Overflow Question for more info.

Instead of changing the prototype you could just write a global function:

function pushIfDoesntExist(array, item) {
 if (array.indexOf(item) === -1) {
 array.push(item);
 return true;
 }
 return false;
}

Weigh Out Pros and Cons before Abstracting

The function pushIfDoesNotExist is well named, so it can act as a comment for what the code is doing. Also, I can think of several of cases where it could be useful. BUT is it really worth writing a function for it? After all, you wouldn't write a function add1:

function add1(number) {
 return number + 1;
}

And that's a well named function with even more applications than pushIfDoesNotExist. What I'm trying to get at here is that abstracting a part of code is not always a good idea. There are some downsides to making a function for something:

1. The function delocalizes code. Someone who is reading through your code to better understand it needs to hunt down where pushIfDoesNotExist is defined to see how it works. (good documentation can help with this)
2. It adds a dependency. Functions that call pushIfDoesNotExist rely on pushIfDoesNotExist being defined properly. (unit tests can help with this)
3. It takes time to write/debug/unit-test/document/maintain/think-about pushIfDoesnNotExist.

Is the value added by the function pushIfDoesntExist really worth the cost? (especially since you only call it once)

Inline Module Constructor

If you want private member variables, or you don't like using this or self you can use an inline function to construct your Testing object.

var Testing = (function () {
 var clickedList = []; //hidden inside inline function scope
 function applyListeners() {
 document.getElementsByTagName("li").forEach(function (node) {
 if (clickedList.indexOf(elem.textContent) === -1) {
 clickecList.push(node.textContent);
 displayData();
 }
 });
 }
 function displayData() {
 document.getElementById("clickedElements").textContent = clickedList.join(", ");
 }
 return {
 applyListeners: applyListeners // public
 };
})();