How to avoid creating several if/statements and/or instances of function

Question

I have this code: https://pastebin.com/zgJdYhzN in Javascript. It's supposed to fade in text when the scrolling function reaches a certain point and while this does work, there will be several pages using it and I'd like to avoid having to create several instances of this function. It would be better if I could just create a function and for every element that had the ".split" class, this would act upon it.

//EXECUTES ONLY ONCE
function once(fn, context) { 
    var result;
    return function() { 
        if(fn) {
            result = fn.apply(context || this, arguments);
            fn = null;
        }
        return result;
    };
}
// Usage
var split1 = once(function() {
    fadeInText(".split1");
});
var pl = once(function() {
    fadeInText(".pl");
});
var pl1 = once(function() {
    fadeInText(".pl1");
});
var smallp = once(function() {
    fadeInText(".smallp");
});
var smallp1 = once(function() {
    fadeInText(".smallp1");
});
var smallp2 = once(function() {
    fadeInText(".smallp2");
});
var smallp3 = once(function() {
    fadeInText(".smallp3");
});
var head0 = once(function() {
    fadeInText(".head0");
});

$(window).scroll(function() {

  if( $(this).scrollTop() + $(window).height() > $(".split1").offset().top) {
      split1();
  }
    if( $(this).scrollTop() + $(window).height() > $(".pl").offset().top) {
      pl();
  }
    if( $(this).scrollTop() + $(window).height() > $(".pl1").offset().top) {
      pl1();
  }
    if( $(this).scrollTop() + $(window).height() > $(".smallp").offset().top) {
      smallp();
  }
    if( $(this).scrollTop() + $(window).height() > $(".smallp1").offset().top) {
      smallp1();
  }
    if( $(this).scrollTop() + $(window).height() > $(".smallp2").offset().top) {
      smallp2();
  }
    if( $(this).scrollTop() + $(window).height() > $(".smallp3").offset().top) {
      smallp3();
  }
    if( $(this).scrollTop() + $(window).height() > $(".head0").offset().top) {
      head0();
  }
});

You really should store `$(this)`, `$(window)`, `$(".split1")`, `$(".pl")`, `...` in variables "outside" (or in a closure) the scroll handler. Right now you're creating 24 jQuery objects (of whom 16 are always the same) on each call of the scroll handler. And that handler will be called _very_ often. — Andreas, Mar 20 '18 at 15:06
I was thinking I could store them in a loop like foreach $('class) and assign an array with its respective index to them. — dan178, Mar 20 '18 at 15:13
Please clarify your requirement/question. In a comment you're saying that you only want to use one class (`.split`) instead of different ones. Then why do you have different functions, one for each class? One class, with one function. That's a simply `for` loop to check the elements... — Andreas, Mar 20 '18 at 17:55

pixelscript · Answer 1 · 2018-03-20T15:36:45.337

1

Not sure if I'm missing why you need the once method. Is there a reason you couldn't do something like this:

var selectors = ['.one', '.two', '.three'];
var elements = {};
selectors.forEach(function(selector){
    elements[selector] = $(selector);
});

function elementOnScreen(selector) {
    if(!elements[selector]){
        return false;
    }
    return  $(window).scrollTop() + $(window).height() > elements[selector].offset().top
}

$(window).scroll(function() {
    selectors.forEach(function(selector) {
        if(elementOnScreen(selector)){
            fadeInText(selector);
            delete elements[selector];
        }
        if(Object.keys(elements).length === 0){
            $(window).off('scroll');
        }
    });
});

edited Mar 20 '18 at 15:36

answered Mar 20 '18 at 15:15

pixelscript

392
1
8

1

you are missing `elements[selector] = undefined` after `fadeInText(selector)` – Jonas Wilms Mar 20 '18 at 15:20
It's because when the scrolltop reaches a certain distance, it executes the function repeatedly but I only need it to fire once. – dan178 Mar 20 '18 at 15:21

Jonas Wilms · Answer 2 · 2018-03-20T15:27:00.017

0

Just generate the functions for all elements using a loop:

 const handlers = [".split1", ".pl" /*...*/]
   .map(s => ({ el: $(s), show: once(() => fadeInText(s)) }));

$(window).scroll(function() {
  for(const {el, show} of handlers) {
    if( $(this).scrollTop() + $(window).height() > el.offset().top) 
       show();
  }
});

You could also generate the handlers for all elements of a class:

const handlers = $(".split").toArray()
   .map(s => ({ el: $(s), show: once(() => fadeInText(s)) }));

edited Mar 20 '18 at 15:27

answered Mar 20 '18 at 15:10

Jonas Wilms

132,000
20
149
151

Do you know how I might adjust this to where I could just use one class for each element that I wanted this to act upon? – dan178 Mar 20 '18 at 15:22
@joshua sure, if `fadeInText` also takes elements – Jonas Wilms Mar 20 '18 at 15:27
I used your revised suggestion, it acted upon the first element but not the others, do you know what's going wrong? [link](https://pastebin.com/kQJ8z8mm) – dan178 Mar 20 '18 at 15:33
and i havent written that code so why should i know whats going on?! – Jonas Wilms Mar 20 '18 at 15:36
I'll upload it to jsbin – dan178 Mar 20 '18 at 15:43
As i said before, in that case `fadeInText` must take jquery elements and not strings – Jonas Wilms Mar 20 '18 at 16:53

How to avoid creating several if/statements and/or instances of function

2 Answers2