1
\$\begingroup\$

I'm no pro in JavaScript. Please help me review this code. I would appreciate any suggestions!

JavaScript:

var y_offset, /*current position of window*/
    distance_from_current_position, /*different between current position and target*/
    body_height, /*hight of body*/
    window_height, /*height of window*/
    position_of_target, /*target position*/
    max_scroll, /*max position to scroll to*/
    up_or_down, /*scroll up or down*/
    my_classes = document.querySelectorAll('.y');

function scroll_function(target) {

    y_offset = window.pageYOffset;

    distance_from_current_position = target - window.pageYOffset;

    setTimeout(function () {

        if (distance_from_current_position !== 0) {
            if (up_or_down) {
                if (y_offset < target - 5) {
                    window.scroll(0, y_offset + (distance_from_current_position / 3))
                } else {
                    window.scroll(0, y_offset + 1)
                }
                if (y_offset < position_of_target - 1) {
                    scroll_function(target);
                }
            } else {
                if (y_offset > target + 5) {
                    window.scroll(0, y_offset + (distance_from_current_position / 3))
                } else {
                    window.scroll(0, y_offset - 1)
                }
                if (y_offset > position_of_target + 1) {
                    scroll_function(target);
                }
            }
        }

    }, 50)
}

function click_function(i) {
    my_classes[i].onclick = function () {
        body_height = document.body.clientHeight;

        position_of_target = document.getElementById(this.getAttribute('data-y')).offsetTop;

        window_height = window.innerHeight;

        max_scroll = body_height - window_height;

        if (window.pageYOffset < position_of_target) {
            up_or_down = 1
        } else {
            up_or_down = 0
        }

        if (position_of_target > max_scroll) {
            position_of_target = max_scroll;
        }
        $y_1(position_of_target);

    };

}

for (var i = 0; i < my_classes.length; i++) {
    click_function(i);
}

HTML:

<!DOCTYPE html>
<title></title>
<ul class=ul>
<li><a class=y data-y=id1>Scroll to id1</a>
<li><a class=y data-y=id2>Scroll to id2</a>
<li><a class=y data-y=id3>Scroll to id´3</a>
</li>
\$\endgroup\$
  • 1
    \$\begingroup\$ What are you trying to solve exactly? It seems cleaner to use built in window.scrollTo() or even using <a href="#elementid">. Sometimes there is a good reason not to use a built in method but sometimes not. \$\endgroup\$ – James Khoury Dec 9 '13 at 23:04
  • \$\begingroup\$ Thanks James, I will add your changes. One thing would like is a smoother easeing effect than (distance_from_current_position / 3). Any idea on how to fixa that? \$\endgroup\$ – user1087110 Dec 10 '13 at 7:10
  • \$\begingroup\$ Again what are you trying to solve exactly? I'd love to post an answer but I can't see any reason for the code. A code-less solution would be much better in my opinion. \$\endgroup\$ – James Khoury Dec 11 '13 at 4:52
2
\$\begingroup\$

From a once over:

  • This code does not work, you did not provide $y_1, I am assuming you meant scroll_function
  • I built a JsBin for this : http://jsbin.com/sihoj/1/edit, because you keep calling scroll_function, you can actually prevent the user from reaching the navigation links again, that's not a good approach
  • As James Khoury mentioned, why not use <a href="#elementid"> or scrollTo ?
  • You should use lowerCamelCase
  • This :

    if (position_of_target > max_scroll) {
        position_of_target = max_scroll;
    }
    

    could be

    position_of_target = Math.min( position_of_target , max_scroll );
    

    I feel this reflects better what you are trying to accomplish

Naming

I would much rather see

//Distance from currrent position
distance = target - window.pageYOffset; 

than

distance_from_current_position = target - window.pageYOffset; 

your code becomes a bit unwieldy when you have too many long variable names

If you have target and would have distance , then I would also go for offset, y_offset looks ugly.

Also here

position_of_target = document.getElementById(this.getAttribute('data-y')).offsetTop;

you should have some deep thoughts about the variable name, position implies usually an x/y coordinate, but you are really dealing here with an offset from the top.

Finally : up_or_down is a terrible name, there is no logical connection for 1 being up, and 0 being down. At the very least you should consider var UP = true, DOWN = false or var UP = 1, DOWN = 0 if you don't want to use booleans.

Magic Constants

/ 3 <- That 3 should be a well named constant
target - 5 <- That 5 should be a well named constant

Fake Booleans

This:

    if (window.pageYOffset < position_of_target) {
        up_or_down = 1
    } else {
        up_or_down = 0
    }

is letting up_or_down be a fake boolean, you should go for

    up_or_down = (window.pageYOffset < position_of_target)

especially since you are doing falsey comparisons to up_or_down any way.

DRY

This:

        if (up_or_down) {
            if (y_offset < target - 5) {
                window.scroll(0, y_offset + (distance_from_current_position / 3))
            } else {
                window.scroll(0, y_offset + 1)
            }
            if (y_offset < position_of_target - 1) {
                scroll_function(target);
            }
        } else {
            if (y_offset > target + 5) {
                window.scroll(0, y_offset + (distance_from_current_position / 3))
            } else {
                window.scroll(0, y_offset - 1)
            }
            if (y_offset > position_of_target + 1) {
                scroll_function(target);
            }
        }

could be

    var vector = up_or_down ? -1 : +1; 

    if (y_offset < target + 5 * vector ) {
        window.scroll(0, y_offset + (distance_from_current_position / 3))
    } else {
        window.scroll(0, y_offset - vector)
    }
    if (y_offset < position_of_target + vector) {
        scroll_function(target);
    }
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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