# Simple star rating system

I'm working on a simple star rating system, and before I move on to actually "tallying" the votes, which I figure I'll just do with $.ajax, I wanted to make sure what I've done so far has been done as well as it can be. JavaScript //max number of stars var max_stars = 5; //add images to the ratings div function fill_ratings() { var$rating = $("#rating"); if($rating.length) {
for(i = 1; i <= max_stars; i++) {
$rating.append('<img id="' + i + '"src="star_default.png" />'); } } } //variables... var hovered = 0; var has_clicked = false; var star_clicked = 0; function handle_ratings() { if(has_clicked == true) { for(i = 1; i <= star_clicked; i++) {$('#rating img#' + i).attr('src', 'star.png');
}
}

$('#rating img').hover(function() { hovered = Number($(this).attr('id'));
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');
}
}
}).click(function() {
//if they click a star, only set the stars to grey that are after it.
star_clicked = Number($(this).attr('id')); has_clicked = true; for(i = 1; i <= star_clicked; i++) {$('#rating img#' + i).attr('src', 'star.png');
//handle the vote here. eventually.
}
}).mouseout(function() {
//if they haven't clicked a star, set all stars to grey.
if(has_clicked !== true) {
$('#rating img').attr('src', 'star_default.png'); } else { //show their rating for(i = 1; i <= max_stars; i++) { if(i <= star_clicked) {$('#rating img#' + i).attr('src', 'star.png');
} else {
$('#rating img#' + i).attr('src', 'star_default.png'); } } } }); }$(function() {
fill_ratings();
handle_ratings();
});


HTML

<div id="ratings"></div>


The code works, but I was wondering if anyone notices any code that I can improve. Also, is there anything wrong with the way it...looks? Should I be commenting more, indenting differently, or any other "formatting" type things?

• Using Namespaces for your global variables as well as methods is a good practice. – user905 Feb 4 '11 at 18:24

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 vars 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

• Thanks for all the info, I'll start looking into converting it to a plugin and updating/fixing everything you mentioned. I accepted your answer, but if anyone has anything to add - please do! – Andrew Jan 30 '11 at 4:21

Some automation can help a lot nowadays: