Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I want to group the two very similar jQuery functions into just one. Any idea?

HTML:

<input id="progress" type="text" class="spinner-input form-control" maxlength="3" readonly value="<?php echo $progreso['progress'] ?>">
<div class="spinner-buttons input-group-btn btn-group-vertical">                   
    <button id="plus" type="button" class="btn spinner-up btn-xs btn-info">                       
        <i class="icon-angle-up"></i>                    
    </button>                     
    <button id="minus" type="button" class="btn spinner-down btn-xs btn-info">                        
        <i class="icon-angle-down"></i>                     
    </button>                  
</div>

jQuery:

$(document).on('click', '#plus', function(e){
    var progress = parseInt( $('#progress').val() ) + 5;
    if ( progress <= 100 ) {
        $('#progress').val( progress );
    }
    else{
        $('#progress').val( 100 );  
    };
});

$(document).on('click', '#minus', function(e){
    var progress = parseInt( $('#progress').val() ) - 5;
    if ( progress >= 0 ) {
        $('#progress').val( progress );
    }
    else{
        $('#progress').val( 0 );    
    };
});
share|improve this question
add comment

3 Answers 3

up vote 4 down vote accepted

First step would be to extract a function that updates the input:

function modifyProgress(delta) {
  var element = $('#progress');
  var progress = +element.val() + delta;
  element.val( isNaN(progress) ? 0 : progress < 0 ? 0 : progress > 100 ? 100 : progress;
);
}

BTW, never ever use parseInt with out its second parameter (radix). Some browsers interpret numbers starting with a leading 0 (zero) as octal, so that an input of "09" throws an error and "010" to returns 8. Better is to use the unary plus operator.

This simplifies the event handler:

$(document).on('click', '#plus, #minus', function(e){
   modifyProgress(5 * (this.id === "plus" ? 1 : -1));
})

EDIT: You should consider storing your progress value somewhere other inside the input such as a data model (see MVC). Keeping data/logic separate from the GUI is always a good thing. It also avoids needing to re-parse the number every time. And you can "hide" the checking of overflow or underflow inside the model.

share|improve this answer
add comment

Introduce a third function that both of the first two call.

function adjustSpinner(value, amount) {
    var progress = parseInt(value,10) + amount;
    if (progress > 100) return 100;
    if (progress < 0) return 0;
    return progress;
}

$(document).on('click', '#plus', function(e){
    $('#progress').val(adjustSpinner($('#progress').val(), 5));
});

$(document).on('click', '#minus', function(e){
    $('#progress').val(adjustSpinner($('#progress').val(), -5));
});
share|improve this answer
    
You forgot converting $('#progress').val() to a number. This will just lead to string concatenation. –  RoToRa Mar 26 at 16:08
    
Good point. Got ahead of myself. Fixed. –  Roger Mar 26 at 16:14
add comment

You could define one function:

function plus_or_minus(event)
{
    var isPlus = $(this).id === '#plus';

    var progress = parseInt($('#progress').val(), 10); 

    if(isPlus)
        progress += 5;
    else
        progress -= 5;

    if(progress > 100)
        progress = 100;
    else if(progress < 0)
        progress = 0;

    $('#progress').val(progress);
}


// And then bind
$(document).on('click', '#plus', plus_or_minus);
$(document).on('click', '#minus', plus_or_minus);

When condensing your code, try to abstract the common parts out to one function - with a simple check against the target element's ID, we can figure out whether to add or subtract. Next, we check progress's value against 100 and 0 to keep it in bounds either way. Finally, apply the value to the #progress element.

Keep in mind that it's a good idea to always pass a base parameter to parseInt - usually the default will be base 10, but there are no promises made when it comes to dealing with as many browsers as there are out there.

PS I haven't tested this, the only part I'm iffy on is the var isPlus = ... as I haven't done jQuery in a while. If that fails to work, maybe try var isPlus = $(this) == $('#plus')

PPS As a bonus, I think jQuery can accept multiple targets for a single on command - research the docs and see if you can bind the function to both elements at the same time

share|improve this answer
1  
Checking the id would be id.this === "plus" (without jQuery) or $(this).attr("id") === "plus" (with jQuery). $(this) == $('#plus') just tries to compare two jQuery objects that may both point to the same element, but they still be two different objects and thus never equal. –  RoToRa Mar 26 at 16:06
    
@RoToRa thanks - I thought maybe the two would be "loosely equal" with the == –  phatskat Mar 26 at 17:14
add comment

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.