0
\$\begingroup\$

I'm a bit clumsy at writing JavaScript. I have some nice code like this:

(function($){
    $.fn.forceNumeric = function(){
        return this.each(function(){
            $(this).keyup(function(){
                if (!/^[0-9]+$/.test($(this).val())){
                    $(this).val($(this).val().replace(/[^0-9]/g, ''));
                }
            });
        });
    };
})(jQuery);

I know what it does and how it works. I can call the function and I see why it works.

For the life of me, I cannot figure out how to use that same elegant style/approach for my Neanderthal function:

function showHideErrorBox(theFriendlyName, check){
     var topLevelDiv,
        numDivsAfter;

        if(check === 0){
        // REMOVE THE OLD ERROR MESSAGE IN THE ERROR BOX
            $('.form-validation-errors').find('div').each(function(){
                if( $(this).html().indexOf(theFriendlyName) != -1 ){
                    $(this).remove();
                }
            });
        }

        //  CHECK WHETHER THE ENTIRE MESSAGE BOX CAN BE REMOVED NOW
        topLevelDiv = document.getElementById('alert-form-errors');
        numDivsAfter = topLevelDiv.getElementsByTagName('div').length;

        if(check === 0){
            if(numDivsAfter <= 1){
                $('#alert-form-errors').hide();
            }

        }else if(check === 1){

            if(numDivsAfter >= 1){
                $('#alert-form-errors').show();
            }    
        }
}

Can anyone help me out? Where do I start? I have Google'd for tutorials, but I have not been able to find something suitable for my needs.

\$\endgroup\$
  • \$\begingroup\$ can you also show the html? there seem to be an awful lot of divs here, I can't quite maek sense of them \$\endgroup\$ – bjelli Aug 31 '13 at 17:17
2
\$\begingroup\$

You could start by minimizing your function calls. like so

ORIGINAL

$.fn.forceNumeric = function(){
    return this.each(function(){
        $(this).keyup(function(){
            if (!/^[0-9]+$/.test($(this).val())){
                $(this).val($(this).val().replace(/[^0-9]/g, ''));
            }
        });
    });
};

OPTIMIZED

$.fn.forceNumeric = function(){
    return this.each(function(){
        $(this).keyup(function(){
            if (!/^[0-9]+$/.test(this.value)){
                this.value = this.value.replace(/[^0-9]/g, '');
            }
        });
    });
};

Just in this little bit you have saved on 3 Function calls.

And on the larger part.

OPTIMIZED

function showHideErrorBox(theFriendlyName, check){
     var topLevelDiv,
         numDivsAfter;

        if(check === 0){
        // REMOVE THE OLD ERROR MESSAGE IN THE ERROR BOX
            $('.form-validation-errors').find('div').each(function() {
                var $this = $(this);
                if( $this.html().indexOf(theFriendlyName) !== -1 ){
                    $this.remove();
                }
            });
        }

        //  CHECK WHETHER THE ENTIRE MESSAGE BOX CAN BE REMOVED NOW
        topLevelDiv = $(document.getElementById('alert-form-errors'));
        numDivsAfter = topLevelDiv[0].getElementsByTagName('div').length;

        if(check === 0 && numDivsAfter <= 1) {
            return topLevelDiv.hide();
        }

        if(check === 1 && numDivsAfter >= 1) {
            return topLevelDiv.show();
        }    
}

You have also saved on 3 Function calls here too;

Just remember that $ is a function and it's better to call a function once than twice right?

if you ever have something like this

$(this).show();
$(this).val('hello');
$(this).hide();

It should be like this

var $this = $(this);
$this.show()
this.value = 'hello';
$this.hide();
\$\endgroup\$
  • \$\begingroup\$ That's awesome! Thank you for getting me started. I really, really appreciate your help, a ton. Although I will accept your answer, I was wondering if you knew of an example that would let me "convert" my showHideErrorBox() function into something that would look like: $.showHideErrorBox = function(){...}? \$\endgroup\$ – Shawn Spencer Aug 30 '13 at 23:59
  • \$\begingroup\$ You're going to have to explain more than that, because i can't work out how it will work with the selected element. like $('#something').showHideErrorBox(); \$\endgroup\$ – iConnor Aug 31 '13 at 0:45
1
\$\begingroup\$
function showHideErrorBox(theFriendlyName, check){
    //remove the old error message in the error box
    if(check === 0){
        $('.form-validation-errors').find('div').each(function(){
            if( $(this).html().indexOf(theFriendlyName) != -1 ){
                $(this).remove();
            }
        });
    }

    //check whether the entire message box can be removed now
    var alertErrors = $('#alert-form-errors');
    var numDivsAfter = alertErrors.find('div').length();
    if(check === 0 && numDivsAfter <= 1){
        alertErrors.hide();
    } else if(check === 1 && numDivsAfter >= 1){
        alertErrors.show();
    }
}
  1. There's no need to declare the topLevelDiv and numDivsAfter variables all the way at the top of the function. It's good practice to not declare variables until you actually use them.
  2. Don't use caps excessively in your comments. Caps should be used rarely, for emphasis.
  3. You should be able to calculate the value for numDivsAfter by using the same find() method you used at the top of the function. Then, instead of calling each(), call length() to get a count of the number of div elements. However, I'm not that well versed in jQuery, so I might be wrong.
  4. Assign $('#alert-form-errors') to a variable so you don't have to repeat its ID.
  5. You can combine the if statements at the bottom of the function for greater clarity.
\$\endgroup\$
  • \$\begingroup\$ Thank you very much for your feedback. That's definitely more elegant than what I had. Is there any way you can think of that I could change my showHideErrorBox() function to something like $.fn.showHideErrorBox = function(){...}? \$\endgroup\$ – Shawn Spencer Aug 31 '13 at 0:31
  • 1
    \$\begingroup\$ @ShawnSpencer I don't know enough about jQuery to know what the significance of $.fn is. It is possible to assign the function to a variable instead of declaring it as a named function, if you prefer: var showHideErrorBox = function(theFriendlyName, check){ ... }. This is (pretty much) the same as declaring a named function, the syntax is just different. \$\endgroup\$ – Michael Aug 31 '13 at 15:17
1
\$\begingroup\$

If your intent is to hide an error box, if there are no errors inside: you can do that with css alone.

div:empty {
    display:none;
}

See http://jsfiddle.net/bjelline/D2P3r/

\$\endgroup\$
  • \$\begingroup\$ Thank you very much. In this case, however, the box is never entirely empty. There's always one div inside the box that states the obvious (There are errors...) and there could be many other errors in that box. So my function simply looks for a specific error message and removes it when the error has been resolved. There could still be additional errors. \$\endgroup\$ – Shawn Spencer Sep 2 '13 at 15:20

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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