2

While this question have been asked before and many already answered it, my question is strictly about the prototype of the newly created functions.

If you read this piece of code, you will understand that it just works. Also here on codepen.

    // main object
    var Foo = {}; 

    // main methods
    Foo.render = {}; // the Render function to populate later

    Foo.start = function(el,ops){
      return new Actions(el,ops);
    }

    // secondary/utility functions
    var Actions = function(el,ops){
      this.el = document.querySelector(el);
      this.ops = ops || {};
      this.prepare(); // this builds the Foo.render functions
      for (var p in this.ops){
        Foo.render[p](this);
      }
    }; 


    // Action methods
    Actions.prototype.prepare = function(){
      for (var p in this.ops) {
        Foo.render[p] = function(that){ // or r[p]
          that.el.style[p] = that.ops[p] + 'px';
        } 
      }
    }

    // init
    var action = new Foo.start('div',{left:15})

    // check
    console.log(Foo.render['left'].prototype);
<div></div>

The problem is the prototype of the newly created function Foo.render['left'] is something like this Foo.render.(anonymous function) {} instead of something like Foo.render.left() {} or something else, and I am experiencing some performance loss because I am unable to access the newly created function's prototype very fast.

Can anyone please shed some light on how to adapt the .prepare() function to create accurate/accessible (I can't chose the right word) prototype functions within the Foo scope?

Thank you.

thednp
  • 4,401
  • 4
  • 33
  • 45
  • All functions do have `Function.prototype` as their prototoype. There's nothing wrong with that. – Bergi Feb 02 '16 at 23:51
  • You should add an `if (!(p in Foo.render))` to that loop so that you're not recreating functions for every new `Actions` instance. – Bergi Feb 02 '16 at 23:53
  • You seem to have the [standard closure in a loop issue](http://stackoverflow.com/q/750486/1048572), but I can't really see what you want to do here or why. – Bergi Feb 02 '16 at 23:54
  • @Bergi thanks for your valuable comments and link. I'm basically trying to avoid a `Foo.render()` function flowing 30+ `if`s, and replace that with a nice object that only holds all functions that are really required. Less code, more performance modularity/extensibility, and generally a more elegant solution. Please take a look at my answer below, seems to be close to the checked answer you linked, do you approve it or know a better solution considering HTML4 browsers support? – thednp Feb 03 '16 at 00:18
  • I see that you're trying to avoid `if`s to check check which properties are present. But then, after setting up multiple `Foo.render[p]` functions, you still go through them in a loop over `ops`. I wonder why you don't simply create a single `render` method, and put the loop inside there? Apart from that, the solution in your answer seems fine (as in "working correctly", not necessarily "well designed"). – Bergi Feb 03 '16 at 00:25
  • The idea is to give `requestAnimationFrame()` a small set of functions to execute rather than a huge function that checks ALL possible CSS props (as in the above example). I know it's not "well done", as you say, but it's the best we can give the browsers atm. – thednp Feb 03 '16 at 00:29
  • So you mean in the other places where you call the `render` methods, you choose them deliberately instead of looping all of them? Ok, then it's fine. – Bergi Feb 03 '16 at 00:42
  • I'm still not sure what your issue with prototypes is ("*I am unable to access the newly created function's prototype very fast*"). Regardless whether you mean `Foo.render.left.prototype` or `Object.getPrototypeOf(Foo.render.left)`, there is absolutely zero reason to access either? – Bergi Feb 03 '16 at 01:25

2 Answers2

1

You will need to capture the value of p in an extra closure scope. Also I would recommend avoiding to overwrite already-existing methods.

Actions.prototype.prepare = function() {
  for (var p in this.ops) {
    if (!(p in Foo.render)) {
      Foo.render[p] = (function(prop) {
        return function(that) {
          that.el.style[prop] = that.ops[prop] + 'px';
        };
      }(p));
    } 
  }
};

or

Actions.prototype.prepare = function() {
  for (var p in this.ops) {
    (function() {
      var prop = p;
      if (!(prop in Foo.render)) {
        Foo.render[prop] = function(that) {
          that.el.style[prop] = that.ops[prop] + 'px';
        };
      }
    }());
  }
}
Bergi
  • 630,263
  • 148
  • 957
  • 1,375
  • In the second version there could be some type errors, but I will check both, so far first seems ok. I'll mark your question anyway since you're so awesome. – thednp Feb 03 '16 at 15:43
  • I would need a `Foo.render[prop] = Foo.render[prop].prototype` or something to improve the access time for these functions, as the `left.prototype` is still `Foo.render.(anonymous function)`, so it doesn't seem to have a name property. – thednp Feb 03 '16 at 16:15
  • 1
    @thednp: That a function has no `name` property does have zero influence on its speed – Bergi Feb 03 '16 at 16:22
  • OK now, tested both the solutions you provided in all cases and they both make total sense with everything else you mentioned before, it's mixing up the `p` without it bound in the context. Thank you so much. – thednp Feb 03 '16 at 17:54
  • I updated my answer, please feel free to comment on the new idea. – thednp Feb 04 '16 at 18:21
0

I think I found a way to make it work better. The following should do, however I'm still curious to know if there is any better solution.

// main object
var Foo = {}; 

// main methods
Foo.render = {}; // the Render function to populate later

Foo.start = function(el,ops){
   return new Actions(el,ops);
}

// secondary/utility functions
var Actions = function(el,ops){
   this.el = document.querySelector(el);
   this.ops = ops || {};
   this.prepare(); // this builds the Foo.render functions
   for (var p in this.ops){
     Foo.render[p](this);
   }
}; 


// Action methods
Actions.prototype.prepare = function(){
   for (var p in this.ops) {
      Foo.render[p] = (function(){ // or r[p]
         return function(that){ 
           that.el.style[p] = that.ops[p] + 'px';
         }
      })(); 
   }
};

// init
var action = new Foo.start('div',{left:15})

// check
console.log(Foo.render['left'].prototype);

UPDATE: I think I found a way to eliminate one of the closures, basically using the p as the function's second attribute, like so Foo.render[p] = function(that,p){} here we go:

// main object
var Foo = {}; 

// main methods
Foo.render = {}; // the Render function to populate later

Foo.start = function(el,ops){
   return new Actions(el,ops);
}

// secondary/utility functions
var Actions = function(el,ops){
   this.el = document.querySelector(el);
   this.ops = ops || {};
   this.prepare(); // this builds the Foo.render functions
   for (var p in this.ops){
     Foo.render[p](this,p); // we include p here
   }
}; 


// Action methods
Actions.prototype.prepare = function(){
   for (var p in this.ops) {
      Foo.render[p] = function(that,p){ // we also include p here
         that.el.style[p] = that.ops[p] + 'px';
      }; 
   }
};

// init
var action = new Foo.start('div',{left:15})

// check
console.log(Foo.render['left'].prototype);
<div></div>

This eliminates the additional closure and brings the function closer to the main thread's scope. Any comment on the update is welcome.

thednp
  • 4,401
  • 4
  • 33
  • 45
  • I hope you're ok with me editing your answer instead of posting my own; feel free to roll it back if not – Bergi Feb 03 '16 at 00:46
  • I'm happy to learn, please feel free to play and I thank you :) – thednp Feb 03 '16 at 00:56
  • I updated my code based on your changes, binding the `p` into the `(function(p) {})(p)` context is not needed, as `that` is the link between the two main object and it seems to slow the access time a bit. – thednp Feb 03 '16 at 01:18
  • Binding `p` is the reason why you added the closure in the first place, right? Without it, when you do `new Actions(…, {left:…, right:…})` a call to `Foo.render.left(…)` would update the `right` property. – Bergi Feb 03 '16 at 01:22
  • You may be right, but the fact that the returned function body uses the `p`, it's creating a new `Closure` object with `p: "left"` so your idea to bind will create an additional `Closure`, I'm not sure if this is good or not, I believe not. – thednp Feb 03 '16 at 01:53
  • You might want to read again [the question](http://stackoverflow.com/q/750486/1048572) and answers I linked above. It's not interesting whether it creates a closure or not (it surely does with the free variable), but *over what*. The loop scope `p` is not enough. – Bergi Feb 03 '16 at 01:56
  • Thanks. I'm looking at it and I understand both question and answer. – thednp Feb 03 '16 at 02:23
  • @Bergi please tell me, what would be this code of mine using this answer here http://stackoverflow.com/a/9782779? – thednp Feb 03 '16 at 15:13
  • I've posted it as an own answer now. Notice that the [`new function` thing is an antipattern](http://stackoverflow.com/a/10406585/1048572) – Bergi Feb 03 '16 at 15:30
  • I'm not exactly sure what "*brings the function closer to the main thread's scope*" shoud mean and how it improves anything. Eliminating the closure doesn't have much benefit either. Anyway, if you are fine with passing the property name `p` as an argument there a) is absolutely no need to create a different method for every property - just use a static function - and b) there's actually not much need of a function at all, you could simply inline it within your loop - eliminating the call does give a speed improvement. – Bergi Feb 05 '16 at 00:35
  • What I mean about the scope is inside a UMD module definition's function body scope, I (as we all) don't want to populate the global scope, and there's where both `Foo` and `Actions` actually are: in the closed scope. – thednp Feb 05 '16 at 01:28