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.

I accidently made a post on StackOverflow, I'm new and wasn't intentional. I got redirected here. One person mentioned that the below test is not right because i'm using the scroll method and the way I was testing is invalid. Anyways if anyone can look at the variations of the code below and give me some pointers. I would really appreciate it. Thanks Rest of the Post below is copy paste.


I'm new so excuse me for any ignorance or misunderstanding on my part.

I decided to run a few test on js functions on jspref to see which method of using function is better suited for this small particular example. To get a better understanding.

LINK to the page with tested functions

Problem:

Getting different results each time, Cold Start, Re-test, Re-Re Test are all coming with different results

First Func - Is just embeded in normally

$(function() {
  var up = $('#horiz_line').offset().top + $('#horiz_line').height() + 25,
      triga = $('.trigger');
  triga.css("display", "none");
  $(window).scroll(function() {
    var down = $(this).scrollTop();
    (down > up) ? triga.fadeIn('fast') : triga.fadeOut('fast')
  });
});

Second Func - Calling a Global Function

$(function() {
  triga.css("display", "none");
  $(window).scroll(function() {
    scrollMenu($(this).scrollTop())
  });
});

function scrollMenu(down) {
  var up = $('#horiz_line').offset().top + $('#horiz_line').height() + 25,
      triga = $('.trigger');
  (down > up) ? triga.fadeIn('fast') : triga.fadeOut('fast')
}

Third Func - Registering/Calling variable func (To test/Learn: I have no idea wt i'm doing)

$(function() {
  $(window).scroll(function() {
    scrollMenu($(this).scrollTop())
  });
  var scrollMenu = function(down) {
      var triga = $('.trigger'),
          up = $('#horiz_line').offset().top + $('#horiz_line').height() + 25;
      (down > up) ? triga.fadeIn('fast') : triga.fadeOut('fast')
      }
});

Fourth Func - Same as Third except Up is called before hand, and registered as a global func. Which i'm guessing is not a good idea if the project gets bigger over time.

// Variable Up Takes up The Global Namespace. Is it better to pre-define it OR have it         inside the scroll Menu anonymous func and get calculated each time the function is run? 
$(function() {
  var up = $('#horiz_line').offset().top + $('#horiz_line').height() + 25;
  $(window).scroll(function() {
    scrollMenu(up, $(this).scrollTop())
  });
  var scrollMenu = function(up, down) {
      var triga = $('.trigger');
      (down > up) ? triga.fadeIn('fast') : triga.fadeOut('fast')
      }
});

What is a good practice and what not.

Thank you in advance.

TLDR:- I have no idea what I'm doing, just trying to learn a little.

share|improve this question
    
To be clear, are you interested specifically in performance (speed)? Can the position of #horiz_line change? If so then the functions will do slightly different things. I imagine the speed differences will be negligible in this case, and the important question is more one of programme design. –  Stuart Jan 22 '13 at 0:06
    
Yeah it can change. I would very much appreciate program design help or anything I can learn from this like implementing the same variable and causing redundancy in the code, etc. –  Dick Grayson Jan 22 '13 at 1:24
    
have you tested these on an actual page? it would be helpful to see what is happening e.g. with jsfiddle. Also, if you test them I think you will see they are doing different things. –  Stuart Jan 22 '13 at 1:34
add comment

2 Answers 2

up vote 1 down vote accepted

Protecting your code

When this project becomes bigger, you might want to wrap this code like in it's own scope similar to how jQuery plugins are packaged. This is to prevent naming collisions, protect your code and portability. This also implies that you should avoid globals. So case #2 is out.

Abstraction vs Performance

The more you abstract and simplify the API, the more code you actually create thus the more processing it will take. Keep your code simple.

Browser Implementation

Browser will also be a factor in performance. A browser with a more powerful implementation of a certain routine will tend to be faster at that routine compared to the others. In Chrome's case, the first 2 tests run best. However, in Firefox, all tests run at almost the same performance.

DOM querying

Now back to the code with case #2 out of the way. In cases #3 and #4, your code is querying the DOM every time the scroll event is triggered. This causes performance degradation. If you have references to static elements, it's better to cache them outside the function rather than being fetched every time.

This code should do the trick as well as a jsPerf test for your case #1 vs my modified code. As you can see, my Firefox 18 performs both operations equally while Chrome 23 performs better with the modified code which proves that browser implementation can make a difference.

$(function() {
      //cache non-changing elements and values
  var horizLine = $('#horiz_line')
    , up = horizLine.offset().top + horizLine.height() + 25
    , triga = $('.trigger')
    , $window = $(window)
    ;

  triga.css('display', 'none');

  //we cached window wrapped in jQuery earlier so we reuse it
  $window.scroll(function(){
    //omitting down variable since it was only used once
    ($window.scrollTop() > up) ? triga.fadeIn('fast') : triga.fadeOut('fast')
  });

});
share|improve this answer
    
Great Advice from you and Stuart. I really appreciate it. I guess the only thing now to ask is limit setting the global name space or like you mentioned "to prevent naming collisions, protect your code and portability. This also implies that you should avoid globals". Would a better approach be something like (function(){ var masterfunc = init: function(){}, step1:function(){}}) Anyways thanks again, I'll take some time to read more about the subject at hand. Have a great day –  Dick Grayson Jan 22 '13 at 2:55
add comment

These four functions are doing different things:

The 1st and 4th set up immediately and down when the user scrolls.

The 2nd and 3rd set both up and down when the user scrolls. (The 2nd seems unlikely to work properly because triga is not defined in the 1st function, so the line triga.css("display", "none"); will result in an error. Only in the 1st will the triga element be initially hidden.)

So they will behave differently if the element #horiz_line changes position between the function being set and the user scrolling.

I'm guessing what you want is to check the position of the horizontal line and the scroll top when the user scrolls. In this case something like the 2nd and 3rd function should work, but it is needlessly complicated to pass the values as parameters and call a function from within a function. Instead you can do either:

$(function() {
  var triga = $('.trigger');
  triga.css("display", "none");
  $(window).scroll(function() {
    var down = $(this).scrollTop(),
        up = $('#horiz_line').offset().top + $('#horiz_line').height() + 25;
    triga[down > up ? 'fadeIn' : 'fadeOut']('fast');
  });
});

or

$(function() {
  var triga = $('.trigger');
  triga.css("display", "none");
  $(window).scroll(scrollMenu);
  function scrollMenu() {
    var down = $(window).scrollTop(),
        up = $('#horiz_line').offset().top + $('#horiz_line').height() + 25;
    triga[down > up ? 'fadeIn' : 'fadeOut']('fast');
  }
});

If, on the other hand, up never changes, then there will be a performance advantage to either defining up as a global variable or passing it as a parameter to the scrolling function (because accessing the DOM is usually costly in terms of performance.)

I think the other aspect of your question is whether there is a performance difference in passing a value as a parameter to a function vs. having it in the scope of an outer function; I don't know but suspect it makes a negligible difference, especially if the parameter is first defined as a variable in the outer function anyway.

More generally: focus on understanding how the code works before worrying about performance optimisation!

share|improve this answer
    
Thank you so much Stuart. I made sure to read everything carefully to fully understand everything you said. First and Sec func both had the "triga" css display to hidden, I completely forgot to remove that from the code since it wasn't needed anymore. As for Up it makes complete sense to what you are saying about DOM is costly, and I should have kept that in mind. Even though I knew about it. I was just thinking that aern't we also suppose to limit the global namespace as well? –  Dick Grayson Jan 22 '13 at 2:43
    
You're running the code inside an anonymous function so it's not polluting the global namespace anyway (except in case 2, where you have one function outside of the $(function...)). If your code got more complex then the scrolling function could e.g. become part of an object that stored some state information such as up; organising your code into a small number of objects (each possibly containing smaller objects) is how you'd avoid having hundreds of variables in the outermost scope. A step further would be to 'package' the code into modules as @JosephtheDreamer suggests. –  Stuart Jan 22 '13 at 11:21
    
Anyway you seem to be mixing up performance concerns with code design generally. Global variables in some cases might make code faster, but are avoided for the reasons already mentioned. Also having loads of 'quasi-global' variables (variables that are within a function scope but used by inner functions without passing the values) usually suggests the code could be reorganised. –  Stuart Jan 22 '13 at 11:28
add comment

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.