up vote 3 down vote favorite

This snippets works, but I think it can be optimized:

/* highlight input, if it is empty */
$(function() {
    var highlight_if_no_val=function () {
        $(this).toggleClass('highlight-input', !$(this).val());
    }
    $('#id_index-EditForm-gpnr').keyup(highlight_if_no_val).each(highlight_if_no_val);
}
);

Is there a simpler solution?

The .each(hightlight_...) is done to set the hightlight after the page loaded (it can be initial empty or initial filled).

share|improve this question

3 Answers

up vote 1 down vote accepted

Your code looks good enough. "optimize" is a vague action. 

1. /* highlight input, if it is empty */
2. jQuery(function ($) {
3.     function toggleHighlightIfEmpty() {
4.         var $this;
5.         $this = $(this);
6.         $this.toggleClass('highlight-input', !$this.val());
7.     }
8.     $('#id_index-EditForm-gpnr').on('keyup.highlight', toggleHighlightIfEmpty).trigger('keyup.highlight');
9. });

Line 2:

I'd use the aliasing form of document.ready to prevent conflicts with other libs

Line 3:

I'd use the function declaration form over variable declaration form for the internal function. It's really a personal preference sort-of-thing. Additionally, using camelCase is relatively standard for JavaScript function names.

Lines 4-6:

I'd store a reference to $this so that you don't keep calling the jQuery factory function. It shouldn't be significant in any way for performance, but it can save some headaches with perens which can be easily misplaced

Line 8:

I'd set the event using a namespaced event name so that it can be triggered and removed simply by calling $(...).off('keyup.highlight'). Instead of passing the function to each again, I prefer to use trigger. If you need backwards compatibility, use bind instead of on.

None of these "optimizations" should lead to code that's in any significant way different as far as performance is concerned. It really comes down to personal preference.

share|improve this answer
@zzzBov Thank your for the code review – guettli Feb 23 '12 at 7:55
up vote 0 down vote

I did not understand the use of .each(highlight_if_no_val). Moreover, you could have written the function as:

$('#id_index-EditForm-gpnr').keyup(
    function () {
        $(this).toggleClass('highlight-input', !$(this).val());
    }
);
share|improve this answer
This ".each(highlight_if_no_val)" is done to set the hightlight-class after page load. Your solution only changes the class after the keyup event. But I want to toogle the class after page load. – guettli Jan 24 '12 at 20:10
$('#id_index-EditForm-gpnr').toggleClass('highlight-input', !$(this).val()).keyup( function () { $(this).toggleClass('highlight-input', !$(this).val()); } ); would also be good, but any change in behavior to occur when empty would need to be made in both the places. In that case, your code snippet would be great. – James Jithin Jan 25 '12 at 5:15
up vote 0 down vote

Just a thought ... with explination:

//  Move your variable function outside of your ready call to make it accessible later
//  if needed, for instance in another script call from a dynamically drawn in view,
//  Alsot, shorten the name so as to save milliseconds on call time, and minutes in
//  coding it (possibly repeatedly) later.
//  I left the name descriptive, but you could really shortin even more
function zero_highlight() {
    $(this).toggleClass('highlight-input', !$(this).val());
}
//  Now begin the ready code. FYI, for others looking at this, you might should have
//  mentioned you were using jQuery (though looking at it with an experienced eye,
//  it appears obvious)
$(function() {
    //  Here again you might really want to consider a name change, call change,
    //  especially in using .each as (i found in personal experience) jquery's 
    //  .each funtion tends to work much more functionally when calling a tagname
    //  or classname as opposed to name multiple items with the same ID
    //  Not to mention, having multiple items with the same ID could be devastating
    //  to your dynamic functionality in later code.
    $('.frm-edit-gpnr').keyup(zero_highlight).each(zero_highlight);
});
share|improve this answer
"shorten the name so as to save milliseconds on call time" .. pretty sure that's not how it works – GGG Feb 11 '12 at 1:33
lol, well every character is counted (from olden days this was a burden) altho now days its not as important – SpYk3HH Feb 11 '12 at 1:59
@SpYk3HH Thank your for the code review – guettli Feb 23 '12 at 7:56

Your Answer

 
or
required, but never shown
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.