I've got a piece of code that takes an element, checks where it is, and if it's beyond a set place in the viewport, make the opacity increase to 1. I've made the code so that it only runs the checks if they're needed, the problem is that the code looks atrocious, and I suspect there's a better way for me to be doing it.

var $wheresThisAt = viewportHeight*4;
        if (scrolled > ($wheresThisAt))
            //there has to bve a better way...
            $('.salafoot').css('opacity', '0'); 
            $('#salamander-1').css('opacity', '1');
            if (scrolled > (($wheresThisAt)+20)){
                $('#salamander-2').css('opacity', '1');
            if (scrolled > (($wheresThisAt)+40)){
                $('#salamander-3').css('opacity', '1');
            if (scrolled > (($wheresThisAt)+60)){
                $('#salamander-4').css('opacity', '1');
            if (scrolled > (($wheresThisAt)+80)){
                $('#salamander-5').css('opacity', '1');
            if (scrolled > (($wheresThisAt)+100)){
                $('#salamander-6').css('opacity', '1');
            if (scrolled > (($wheresThisAt)+120)){
                $('#salamander-7').css('opacity', '1');
            if (scrolled > (($wheresThisAt)+140)){
                $('#salamander-8').css('opacity', '1');
            if (scrolled > (($wheresThisAt)+160)){
                $('#salamander-9').css('opacity', '1');
            if (scrolled > (($wheresThisAt)+180)){
                $('#salamander-10').css('opacity', '1');
            if (scrolled > (($wheresThisAt)+200)){
                $('#salamander-11').css('opacity', '1');
            if (scrolled > (($wheresThisAt)+220)){
                $('#salamander-12').css('opacity', '1');
            if (scrolled > (($wheresThisAt)+240)){
                $('#salamander-13').css('opacity', '1');
            if (scrolled > (($wheresThisAt)+260)){
                $('#salamander-14').css('opacity', '1');
            if (scrolled > (($wheresThisAt)+280)){
                $('#salamander-15').css('opacity', '1');
            if (scrolled > (($wheresThisAt)+300)){
                $('#salamander-16').css('opacity', '1');
            if (scrolled > (($wheresThisAt)+320)){
                $('#salamander-17').css('opacity', '1');
            if (scrolled > (($wheresThisAt)+340)){
                $('#salamander-18').css('opacity', '1');
            if (scrolled > (($wheresThisAt)+360)){
                $('#salamander-19').css('opacity', '1');
            if (scrolled > (($wheresThisAt)+380)){
                $('#salamander-20').css('opacity', '1');
            if (scrolled > (($wheresThisAt)+400)){
                $('#salamander-21').css('opacity', '1');
            if (scrolled > (($wheresThisAt)+420)){
                $('#salamander-22').css('opacity', '1');
            if (scrolled > (($wheresThisAt)+440)){
                $('#salamander-23').css('opacity', '1');
            if (scrolled > (($wheresThisAt)+460)){
                $('#salamander-24').css('opacity', '1');
            if (scrolled > (($wheresThisAt)+480)){
                $('#salamander-25').css('opacity', '1');
            if (scrolled > (($wheresThisAt)+500)){
                $('#salamander-26').css('opacity', '1');
            if (scrolled > (($wheresThisAt)+520)){
                $('#salamander-27').css('opacity', '1');
            if (scrolled > (($wheresThisAt)+540)){
                $('#salamander-28').css('opacity', '1');
            if (scrolled > (($wheresThisAt)+560)){
                $('#salamander-29').css('opacity', '1');
            if (scrolled > (($wheresThisAt)+580)){
                $('#salamander-30').css('opacity', '1');
            if (scrolled > (($wheresThisAt)+600)){
                $('#salamander-31').css('opacity', '1');
            if (scrolled > (($wheresThisAt)+620)){
                $('#salamander-32').css('opacity', '1');
            if (scrolled > (($wheresThisAt)+640)){
                $('#salamander-33').css('opacity', '1');
            if (scrolled > (($wheresThisAt)+660)){
                $('#salamander-34').css('opacity', '1');
            if (scrolled > (($wheresThisAt)+680)){
                $('#salamander-35').css('opacity', '1');
            if (scrolled > (($wheresThisAt)+700)){
                $('#salamander-36').css('opacity', '1');
            }}}}}}}}}}}}}}}}}}}}}}}}}}}}}}}}}}} // close all if statements before if (scrolled > ($wheresThisAt))
        } // end if (scrolled > ($wheresThisAt))

Here's a sample of the HTML that's being affected by the code

<div class="salafoot" id="salamander-8" style="opacity: 0;"></div>

As you can see I reuse almost identical code a lot of times, and if I want to change the number of steps (say to 37) I need to add another line. The if statements all get left open until the end, so that it only runs the check on the next line if it's useful (only checks +40 if the +20 has already been approved). All of the #salamander-(n) IDs are tied to the .salafoot class. The expected output is footprints that appear one at a time as you scroll down the page.


5 Answers 5

var top  = Math.max(Math.min(Math.ceil((scrolled-$wheresThisAt)/20),35),1); 

for (var i = 1; i < top; i++) {
    $('#salamander-' + i).css('opacity', '1');

This is similar to Amon's answer, but avoids the if structure.

  • 10
    \$\begingroup\$ That long combination of Math.max, Math.min, Math.ceil on the same line I don't like. Otherwise a good approach. Using some additional variables would help make that long line more readable. \$\endgroup\$ Mar 28, 2014 at 13:08

Gah! Thanks for coming here.

Essentially, all your if-statements have the following structure:

if (scrolled > (($wheresThisAt)+(N * 20))){
    $('#salamander-(N + 1)').css('opacity', '1');

So we can comfortably roll that into a loop:

for (var i = 0; i < 35; i++) {
    if (scrolled <= ($wheresThisAt + i * 20)) {
    $("#salamander-" + (i + 1)).css('opacity', '1');
  • 2
    \$\begingroup\$ That's some terrible abuse of break and if, not to mention you can do it without an if structure. \$\endgroup\$
    – Kvothe
    Mar 28, 2014 at 11:43
  • 4
    \$\begingroup\$ @Kvothe: Isn't that a guard clause? blog.codinghorror.com/flattening-arrow-code \$\endgroup\$
    – palacsint
    Mar 28, 2014 at 12:03
  • 4
    \$\begingroup\$ @Kvothe (1) I see nothing wrong with break. While I'd personally prefer functional programming, JS is at its heart an imperative language. (2) I see no “terrible abuse” of anything in my code, except maybe magic numbers. (3) It was intended as a simple refactoring of the original code – I applied the rule “three or more: use a for”, and swapped > for <= with an “early return“, except that return is a break in the context of a loop. I decided not to use excessive cleverness which does make the if unnecessary. I don't quite understand your criticism. \$\endgroup\$
    – amon
    Mar 28, 2014 at 12:15
  • 7
    \$\begingroup\$ @Kvothe bad practice? I don't think so. Of course the alternative would be for (var i = 0; i < 35 && scrolled > ($wheresThisAt + i * 20); i++), but that's far less readable. The way I wrote it in my answer makes it clear that we are primarily iterating i over 035, and that we'll stop once we've scrolled too far. \$\endgroup\$
    – amon
    Mar 28, 2014 at 12:34
  • 2
    \$\begingroup\$ Passion abound this morning! Not abuse in my mind, but I would still have reversed the condition and put the .css() in the if block. Easier to grok that way. \$\endgroup\$
    – konijn
    Mar 28, 2014 at 14:44

You could easily loop over the element names...

for (var i = 2; i <= 36; ++i) {
    if (scrolled > ($wheresThisAt + (i-1)*20)) {
        $('#salamander-' + i).css('opacity', 1);
    else break;

But if they're all defined sequentially in the page as i'm assuming they are, you could assign them a CSS class and then say something like

var shownCount = Math.max(Math.floor((scrolled - $wheresThisAt) / 20), 0) + 1;
$('.theirClass').slice(0, shownCount).css('opacity', 1);

and avoid the loop altogether (on your end, anyway).

  • 1
    \$\begingroup\$ else break;, what...? Change the conditional of an if loop, break is just a glorified goto. \$\endgroup\$
    – Kvothe
    Mar 28, 2014 at 11:44
  • 1
    \$\begingroup\$ @Kvothe: Yeah. there's actually a bunch of nested if statements. :P It's not strictly necessary to skip to the end, but that's what the code it's replacing is basically doing. And there's no good reason to keep looping, or to go through the contortions required to satisfy the anti-break cargo cultists. \$\endgroup\$
    – cHao
    Mar 28, 2014 at 11:47
  • \$\begingroup\$ @cHao I'd call that premature optimization... this is only relevant when processing large sets of data, and ~40 items is definitely not a lot. \$\endgroup\$
    – Vogel612
    Mar 28, 2014 at 12:28
  • 4
    \$\begingroup\$ @Kvothe - break is a glorified GOTO, in the same way that a while loop, if statement, and for loop are all also glorified GOTO. What sets them apart is the target of the jump.... not the source. GOTO is ugly because the target of the jump is unconstrained. break is great because the target of the jump is a well-defined and optimized code-point. See what Knuth has to say about it, and realize that 'break' and 'continue' are the good kind of 'goto'. \$\endgroup\$
    – rolfl
    Mar 28, 2014 at 12:47
  • 1
    \$\begingroup\$ @Vogel612: I wasn't even talking about performance. If you know there won't be any more results, there's simply no good reason to continue. If you're continuing because you haven't realized that there's no further work to do, OK. But if you do realize it, and you're continuing solely to avoid a "glorified goto", that's boneheaded. \$\endgroup\$
    – cHao
    Mar 28, 2014 at 13:33

You're already getting all the elements to set the initially opacity: $('.salafoot').css('opacity', '0');

You may as well set the opacity for each item so you're only going over them once (this assumes they are in sequential order).

$('.salafoot').css('opacity', function(i, elem){
    return scrolled > $wheresThisAt+20*(1+i) ? '1' : '0';

I think @amon's and @Kvothe's solutions can be simplified a bit into something which is simple and readable at the same time:

var i = 0;
while (i < 35 && $wheresThisAt + i * 20 < scrolled) {
    $("#salamander-" + (i + 1)).css('opacity', '1');

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.