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.

200_success
145k22 gold badges190 silver badges478 bronze badges
asked Aug 30, 2013 at 20:14
\$\endgroup\$
1
  • \$\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\$ Commented Aug 31, 2013 at 17:17

3 Answers 3

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();
answered Aug 30, 2013 at 20:53
\$\endgroup\$
2
  • \$\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\$ Commented Aug 30, 2013 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\$ Commented Aug 31, 2013 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.
answered Aug 30, 2013 at 20:55
\$\endgroup\$
2
  • \$\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\$ Commented Aug 31, 2013 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\$ Commented Aug 31, 2013 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/

answered Aug 31, 2013 at 17:26
\$\endgroup\$
1
  • \$\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\$ Commented Sep 2, 2013 at 15:20

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

By clicking "Post Your Answer", you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.