Simple star rating system

Question

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?

Answer 1

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

Answer 2

Some automation can help a lot nowadays:

Google's own Closure Compiler from Google

is a tool for making JavaScript download and run faster. It parses your JavaScript, analyzes it, removes dead code and rewrites and minimizes what's left. It also checks syntax, variable references, and types, and warns about common JavaScript pitfalls.

There's a online version of this tool too, a Firebug plugin, and is already part of PageSpeed and there's also a JavaScript Linter as part of these Closure Tools.