CODEWITHSUNDEEP
jquery
Ricardo Zea
Ricardo Zea

Reputation: 10283

jQuery: Optimize code (learning case)

I have to admit, I came up with this solution by pure intuition from seeing many jQuery scripts during all the time I've been experimenting and learning on my own.

Since I don't have anyone to ask in person these kind of questions, that's why I come here for help and guidance.

--

Inspired by Wufoo's plans' forms where the row you need to focus is highlighted while filling out the form, I created this script (I am not a jQuery or JavaScript guru, I'm still learning and practicing):

//Improve usability by highlighting the row the user needs to focus on:
$(function() {
        //When input element has focus
        $('input').focus(function(){
            //Add the class to its corresponding row
            $(this).parent().parent('.row').addClass('focus'),
                //And when it loses focus
                $('input').blur(function(){
                    //Remove the class
                    $(this).parent().parent('.row').removeClass('focus');
                });
        });
});

So I was wondering if this script has a way of being written better or optimized/shorten in any way. If not, and the way I wrote it's Ok, that's fine, all I'd like to learn is ways of optimizing code when possible, that's all.

I created a Demo in Codepen if you want to check it out.

Thanks in advance.

Upvotes: 0

Views: 96

Answers (3)

Jani Hyytiäinen
Jani Hyytiäinen

Reputation: 5407

I would try to keep my even handlers and event binding separated. This is convenient to do with the .on() which actually binds the event to all the current and future .row elements unlike the .focus() which only binds the event on the rows you had on the page when the binding happened.

This way you don't need to wrap your code into $(function(){}); either. And you can have the event binding in an included .js file instead of mixing your jQuery with your markup.

Additionally, use .closest() instead of .parent().parent() because it won't break if you choose to change your structure a bit. Provided that your .row is still there.

var onRowFocus = function(){
    $('.row').removeClass('focus');
    $(this).closest('.row').addClass('focus');
};

$('.row').on('focus', 'input', onRowFocus);

Also, you don't need to bind the .blur event at all. Simply remove the focus class from all rows before you add it to the one on focus. Less event handlers, less code, easier to maintain.

Upvotes: 1

Niels
Niels

Reputation: 49919

You can do this:

//Improve usability by highlighting the row the user needs to focus on:
$(function() {
        $('input').focus(function(){
            $(this).closest('.row').addClass('focus'); // Change here                    
        }).blur(function(){ // Change here
           $(this).closest('.row').removeClass('focus'); // Change here
        });
});

Or even shorter!

$(function(){
    $("input").on("focus blur", function(){
        $(this).closest(".row").toggleClass("focus");
    });
});

You did 1 thing totally wrong!! Each time you focus you bind a new blur to each input!

Upvotes: 4

Rory McCrossan
Rory McCrossan

Reputation: 337560

Code looks good. The only changes I'd make are to use closest() instead of chaining parent() calls, as if you change the DOM around you would need to change your jQuery code. Also, I'd move the blur() handler outside of the focus() handler. Try this:

$(function() {
    $('input')
        .focus(function() {
            $(this).closest('.row').addClass('focus');
        })
        .blur(function() {
            $(this).closest('.row').removeClass('focus');
        });
});

Upvotes: 4

Related Questions