Skip to main content
Code Review

Return to Answer

Commonmark migration
Source Link

##Take care extending built in objects

Take care extending built in objects

##Rewrites

Rewrites

##Take care extending built in objects

##Rewrites

Take care extending built in objects

Rewrites

Source Link
Blindman67
  • 22.8k
  • 2
  • 16
  • 40

##Take care extending built in objects

  • Extending the prototype of builtin objects should be done with care. The best way is via Object.defineProperty and first check if something has not already done so. (See 2nd rewrite)

  • Why create two copies of this.. self and that. Why not just use this and create a map object with a meaningful name. (see 2nd rewrite)

  • Don't add functionality outside the role of the function. Array.unique Defiantly should not be calling an alert (in fact you can not trust alerts (clients can turn them off) so you should never use them)

  • You have actually repeated the process of removing duplicates in the keypress handler. You could just have checked if the length of data and result are different and alerted the client from that. (see 1st rewrite)

##Rewrites

First quick rewrite showing you don't need Array.unique

$(document).ready(function(){
 const lol = $('#lol');
 lol.keypress(function(){
 const data = lol.val().split('\n');
 const result = data.filter((word, i, arr) => arr.indexOf(word) === i);
 lol.val(result.join('\n'));
 result.length !== data.length && (alert("duplicate"));
 });
});

The second rewrite showing a safe way to add to inbuilt prototypes.

Note that the options configurable, enumerable, writable are set to false. Particularly you MUST set enumerable to be false or you could end up with all sorts of bugs appearing in existing code.

You should also learn to not use jQuery. It has had its day and will just set you back as it shelters you from learning modern JS and DOM interfaces.

"use strict";
lol.focus();
lol.addEventListener("keyup", event => {
 const data = lol.value.split("\n");
 const result = data.unique();
 info.textContent = result.length !== data.length ? "Duplicate removed" : "";
 lol.value = result.join('\n'); 
});
if (!Array.prototype.unique) {
 Object.defineProperty(Array.prototype, "unique", {
 configurable: false, // these 3 properties default to false so not needed
 enumerable: false, // But have added them just to show their availability.
 writable: false, 
 value: function() { 
 const existing = {};
 return this.filter(a => existing[a] = !existing[a] ? true : false);
 }
 });
} else {
 throw new Error("Array.prototype.unique already defined.");
}
<textarea id="lol"></textarea>
<div id="info" style="color:red"></div>

default

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