Skip to main content
added 284 characters in body
Source Link
Yi Jiang
  • 520
  • 4
  • 13

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

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.

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

Source Link
Yi Jiang
  • 520
  • 4
  • 13

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.