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'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.

share|improve this question
add comment

5 Answers 5

up vote 12 down vote accepted
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.

share|improve this answer
9  
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. –  Simon André Forsberg Mar 28 at 13:08
add comment

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)) {
        break;
    }
    $("#salamander-" + (i + 1)).css('opacity', '1');
}
share|improve this answer
2  
That's some terrible abuse of break and if, not to mention you can do it without an if structure. –  Kvothe Mar 28 at 11:43
4  
@Kvothe: Isn't that a guard clause? blog.codinghorror.com/flattening-arrow-code –  palacsint Mar 28 at 12:03
4  
@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. –  amon Mar 28 at 12:15
7  
@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. –  amon Mar 28 at 12:34
2  
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. –  konijn Mar 28 at 14:44
show 3 more comments

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).

share|improve this answer
1  
else break;, what...? Change the conditional of an if loop, break is just a glorified goto. –  Kvothe Mar 28 at 11:44
1  
@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. –  cHao Mar 28 at 11:47
    
@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. –  Vogel612 Mar 28 at 12:28
4  
@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'. –  rolfl Mar 28 at 12:47
1  
@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. –  cHao Mar 28 at 13:33
add comment

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';
});
share|improve this answer
add comment

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');
    i++;
}
share|improve this answer
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.