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>
1 Answer 1
From a once over:
- This code does not work, you did not provide
$y_1, I am assuming you meantscroll_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">orscrollTo? - 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);
}
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\$