The obvious improvement to the code you have here is that you should package it up as a jQuery plugin, to improve code reusability and reduce the number of global variables created.
You'll also want to use a class
instead of id
, to improve reusability (if you chose not to change this to a plugin), and to store as much information on the element themselves, instead of using global variables to store information, to create less coupled functions.
Otherwise, going down the code line by line:
Inside the fill_ratings
function:
$rating.append('<img id="' + i + '"src="star_default.png" />');
You are creating elements with id
attribute starting with a number. This is illegal before HTML5. Also, you can create elements in a more 'jQuery' manner with this syntax instead:
$('<img>', {
id: 'star-' + i,
src: 'star_default.png'
}).appendTo($rating);
The main benefit of this is that now you have a reference to element you just created. You can then push it to an array, to reuse later, instead of creating new jQuery objects by attempting to select the same elements again later in the code.
Variable declaration - mostly a style suggestion, but you can avoid multiple var
s by using the comma operator:
var hovered = 0,
has_clicked = false,
star_clicked = 0;
Don't use $(this)
to get element attributes:
$(this).attr('id');
is about 97% slower than
this.id;
the latter is less verbose and much much more efficient.
Inside each of the event handlers attached to #rating img
- instead of looping through all #rating img
element, like you're doing:
for (i = 1; i <= max_stars; i++) {
if (i <= hovered) {
$('#rating img#' + i).attr('src', 'star.png');
} else {
$('#rating img#' + i).attr('src', 'star_default.png');
}
}
We can always use the information already given (the current element triggering the event) to do the processing. The above code, for instance, can be written as:
$(this).prevAll().attr('src', 'star.png');
thereby reducing the all those lines into one, and removing a variable. Most of the other event handlers can be similarly rewritten.
Type coercion to Number
can be done without using Number
- just append a +
in front of the variable. In your case, it is completely unnecessary, since you're coercing it back to String
again after joining it to another string in the selector passed to the jQuery selector