Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 2 down vote favorite

I'm currently working on a pretty easy jQuery element. But I am still learning about jQuery so I thought maybe Stackoverflow can show me something I need to learn.

You can see the element here: http://jsfiddle.net/4UbRU/

When you click on a span it enters the value into the input. That works fine, but as you can see the js code is very big and there must be a better way to do this, but like I said I'm still learning.

Maybe someone can give me a pointer in the right direction.

ps, I supose I have to use a variable somewhere but can't really figure it out on my own.

addede the jQuery code.

jQuery(document).ready(function() {

    $("#spanval1").click(function(){
        $('#hourvalue').val($('#spanval1').text());
    });

    $("#spanval2").click(function(){
        $('#hourvalue').val($('#spanval2').text());
    });

    $("#spanval3").click(function(){
        $('#hourvalue').val($('#spanval3').text());
    });

    jQuery('.hour_dropdown').hide()

    jQuery("#more").click(function() {
        $('.hour_dropdown').fadeToggle(200);
    });
});​
share|improve this question
Thank you for noting that, added the jQuery code to the post. – alucardu Aug 6 '12 at 13:44

3 Answers

up vote 4 down vote accepted

Building off of Danny's answer, if you have access to the HTML, you can add a class to each of your span tags and bind an event to that class:

<span id="spanval1" class="spanval">1</span>
<span id="spanval2" class="spanval">2</span>
<span id="spanval3" class="spanval">3</span>

The event call can become more generic by using the event's local context 'this'. From the .bind() documentation:

Within the handler, the keyword this refers to the DOM element to which the handler is bound. To make use of the element in jQuery, it can be passed to the normal $() function.

'this' inside of the click event will be the DOM element that was clicked on that caused the event to fire. Then you would need only the following to setup the click event for all of the elements with the class 'spanval':

$(".spanval").click(function(){
    $('#hourvalue').val($(this).text());
});

And with the rest of the script:

$(document).ready(function() {

   $(".spanval").click(function(){
       $('#hourvalue').val($(this).text());
   });

   $('.hour_dropdown').hide();

   $("#more").click(function() {
       $('.hour_dropdown').fadeToggle(200);
   });

});​
share|improve this answer
Typo alert: The first selector there should end with the letter L. – Joseph Silber Aug 6 '12 at 21:19
Thanks. But can you perhaps explain me how (this) knows the value of the span? – alucardu Aug 6 '12 at 22:20
up vote 0 down vote

Here are some points worth considering:

  1. Cache your selectors! If there's one thing you can do to increase performance, it's caching selectors. Traversing the DOM is an expensive operation, and should be done only when absolutely necessary.

  2. Use an attribute selector (span[id^=spanval]). This'll allow you to select all spans who's ID starts with spanval (if not all of them are spans, you can use the attribute selector on its own ([id^=spanval]) which is a tad slower but works just as well).

  3. Whenever possible, try not to use the jQuery constructor. It's much faster to call the static methods from the jQuery namespace (when they're available). So instead of $(this).text(), try using $.text(this).

With that in mind, here's some sample code:

jQuery(document).ready(function($) {

    var $hourVal = $('#hourvalue'),
        $hourDropdown = $('.hour_dropdown');

    $('span[id^=spanval]').click(function(){
        $hourVal.val( $.text(this) );
    });

    $hourDropdown.hide()

    $("#more").click(function() {
        $hourDropdown.fadeToggle(200);
    });
});​
share|improve this answer
up vote 2 down vote

First off, this refers to the element you clicked on, so we can simplify your code a bit my replacing your second #spanval like so:

$("#spanval1").click(function(){
    $('#hourvalue').val($(this).text());
});

Next, you can extract that click handler into it's own function, allowing the index to be passed in as a parameter.

function handleClick(num){
    $("#spanval"+num).click(function(){
        $('#hourvalue').val($(this).text());
    });
}

This will allow us to replacing all of those click handlers in the main function with this:

handleClick(1);
handleClick(2);
handleClick(3);

Now, we're still being repetitive, so lets reduce the repetition using a for loop:

jQuery(document).ready(function() {

    var maximumNumber = 3;
    for(var n = 1; n <= maximumNumber; n++){
        handleClick(n);
    }

    jQuery('.hour_dropdown').hide()

    jQuery("#more").click(function() {
        $('.hour_dropdown').fadeToggle(200);
    });
});​
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.