current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 0 down vote favorite

So I've creating a small library of JavaScript functions that I will be using. Some are for UI functions others are to return something. Below I will post two codes, I am wondering if this method is ok and not a bad move before going any further with this library, also please do not mention jQuery as this is all JavaScript!

HTMLElement.prototype.drag = function(bool) {   
var self = this;
self.classList.add('draggable');
    event.stopPropagation();
    event.preventDefault();
    var origLeft= parseInt(self.offsetLeft,10);
    var origTop = parseInt(self.offsetTop,10);
    var mdX = event.clientX;
    var mdY = event.clientY;
    var elements = [];
    if(bool === true){
      var j = window.localStorage.getItem('userSettings');
      if(j){
        self.style.left= j.left;
        self.style.top = j.top;
      }
    }
      function drag(bool) {
        self.style.position="absolute";
        self.style.zIndex=999;
        self.style.left = origLeft + event.clientX - mdX + "px";
        self.style.top = origTop + event.clientY - mdY + "px";
        event.stopPropagation();
      }
      function stop(bool){
        self.classList.remove('draggable');
        self.style.zIndex=0;
        document.removeEventListener('mousemove',drag,true);
        document.removeEventListener('mouseup',stop,true);
           if(bool === true){
            var settings = {
              left: self.style.left,
              top: self.style.top
            };
            var b = JSON.stringify(settings);
            window.localStorage.setItem('userSettings',b);
            console.log(b);
            }
        event.stopPropagation();
      }
       document.addEventListener('mousemove',drag,true);
       document.addEventListener('mouseup',stop,true);  
};

The above code is of course for a draggable function, below is a getClientRects() sort of function but better. 

HTMLElement.prototype.getRect = function(){
     var a = this.offsetLeft;
     var b = this.offsetTop;
     var c = this.offsetHeight + b;
     var d = this.offsetWidth + a;
     var f = this.offsetHeight;
     var g = this.offsetWidth;
     var obj = { };
     obj.left = a;
     obj.right=d;
     obj.top=b;
     obj.bottom=c;
     obj.width = g;
     obj.height=f;
  return obj;
};

Purpose of showing you all this is to get your thoughts and maybe advice about prototype or should I not do it this way. I just like the method execution like this document.getElementById('div').getRect(); instead of doing it with a function getRect('div');

I originally asked this at stackoverflow though they suggested to move it here. Some of the following comments proceeded.

Extending host objects is not a good idea, see What’s wrong with extending the DOM.

Making them enumerable is quite bad, as that makes it show up when iterating using a for-in loop.

If done carefully extending natives can be ok; javascriptweblog.wordpress.com/2011/12/05/

Problem is for one not sure what enumerable is as I've self taught everything, and never really read on the terminology. I know what one of the users means when he says iterating them using a for in loop though not sure how to make it non-enumerable. Nor how to go about this any better way as I don't want my users (who use jQuery alot) though the library is huge and honestly I'd rather them use a vanilla JavaScript library in the sense that is minimized for purposes of what I am doing. (Making a huge plug-in with a library and other coding options they can have their hands on)

Any suggestions and remarks are welcomed. Please just don't downvote I am only here to learn from my mistakes and feed off your knowledge thank You

share|improve this question
    
There is no need to make is enumerable...just use defineProperty to add functionality. –  vsync Aug 10 at 13:02

2 Answers 2

up vote 1 down vote accepted

I don't have any insight on extending host objects, because I've never done it, and quite frankly this scares me a little bit.

That said, I can shed a bit of light on that "enumerable" problem.

Objects in javascript are traversable, much like arrays. This means that when you do :

var o = {foo: "bar", baz: "buzz"};
for(var property in o){
  console.log(o[property]);
}

you get "bar" and "buzz" in the console.

Now in a for-in loop, the only properties that you get are those who are enumerable. What does this mean ? Basicly, this means that you only get properties for which o.propertyIsEnumerable('propertyName') is true (and by extension, for which o.hasOwnProperty('propertyName') is true).

When you create a property on an object, by default it is enumerable, and hasOwnProperty returns true for it. But it is possible to define properties with modifiers :

   o = {};
   Object.defineProperty(o, 'foo', {
     value:      function(){ console.log('foo'); },
     enumerable: false
   });

   o.foo();                      // "foo"    
   o.propertyIsEnumerable('foo') // false
   o.hasOwnProperty('foo')       // false 
   for(property in o){
     console.log(property); // this will never be reached - o has no enumerable property yet
   }

One reason to mark certain properties as not enumerable is that when extending an object, one usually copies all enumerable properties from a source object using a for-in loop. When writing a lib, you have to care about this, because if you don't your users will have to deal with properties they probably are not aware of or don't care about.

Also note that every fresh object already has non enumerable properties : prototype, hasOwnProperty, etc...

Hope this helps. For more info, see Object.defineProperty

share|improve this answer
    
Ok so I have to use Object.defineProperty(o being the object, foo being the property of o and value is the properties value and then enumerable false makes it non-enumerable. Do I have to do this to each property, hence defineProperty and not defineProperties –  EasyBB Nov 6 '13 at 16:14
    
no, see... Object.defineProperties –  m_x Nov 6 '13 at 18:20
    
Ok thank you, I'll read on it. If all is well then I'll mark this as the accepted answer. –  EasyBB Nov 6 '13 at 23:34
    
I understand now. Though I am now getting in FF something about property only has a getter or something like that –  EasyBB Nov 7 '13 at 4:14
up vote 1 down vote

My suggestions on whether to extend native objects or not:

I would not do it. Here are some reasons based on your specific code:

  • Lets say browsers decide to implement their own "drag" method in the future. If this happens and your drag function doesn't match theirs, then you'll have the confusing case where your code differs from the standard Javascript interface any other developers know and use. Your code would also be hiding the functionality of the browser's new "drag" method, which may be completely different.

  • A problem specific to how you are extending DOM elements: different browsers have different interfaces to the DOM and certain DOM prototypes may not be available in certain browsers. This is described in this article: http://perfectionkills.com/whats-wrong-with-extending-the-dom/. For instance, Internet Explorer 7 does not expose DOM elements like Node or Element. Although you are not touching Node or Element, it would be easy to make this mistake if you being extending DOM elements.

share|improve this answer
    
your first argument is not an issue since you can check if such functionality is previously declared, then do not proceed with your custom one. Your second argument had more weight in the past, but nowadays things are much more stable. –  vsync Aug 10 at 13:05

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.