Skip to main content
Code Review

Return to Answer

replaced http://stackoverflow.com/ with https://stackoverflow.com/
Source Link

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 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
 };
})();

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
 };
})();

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
 };
})();
Source Link
Joshua Dawson
  • 1.9k
  • 11
  • 20

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
 };
})();
lang-html

AltStyle によって変換されたページ (->オリジナル) /