jQuery hide and show option value from a dropdown

Question

I have form with an "add field" button that does an Ajax call to add a new field.

Each field contains a list of options to choose from. When is public is Yes, I need to remove option value = "contacts" and "file upload" from the drop and when the "Add field" button is clicked. If the is public is no, I need to show the option value "contacts" and "file upload".

My code works, but I feel I can improve it.

enter image description here

publicFormValidation = function(){
 $('#public').change(function(){
 if(checkForm()){
 $('.field_type option[value="file_upload"]').hide();
 $('.field_type option[value="contacts"]').hide();
 $('.add_many_fields').on('click',function(){
 setTimeout(function(){
 $('.field_type option[value="file_upload"]').hide();
 $('.field_type option[value="contacts"]').hide();
 }, 500);
 }); 
 }else {
 var $contacts = $('.field_type option[value="contacts"]');
 var $file_uploads = $('.field_type option[value="file_upload"]');
 $contacts.show();
 $file_uploads.show();
 $('.add_many_fields').on('click',function(){
 setTimeout(function(){
 var $contacts = $('.field_type option[value="contacts"]');
 var $file_uploads = $('.field_type option[value="file_upload"]');
 $contacts.show();
 $file_uploads.show();
 }, 500);
 });
 };
 });
}
checkForm = function(){
 var public_val = $('#public').val();
 return (public_val == "true");
}

The reason I have a setTimeout function is because when I needed to create a delay from when click to hide onto the DOM.

Answer 1

The primary things I would recommend here are

1. avoid polluting the global namespace
2. avoid unintentionally creating globals
3. cache your DOM element lookups to increase performance and also decrease filesize

Here is the code reflecting these considerations:

// Wrap this in a self-executing, anonymous scope, or in a document.ready
// this way you can avoid polluting the global (window) namespace
(function(){
 // Cache this DOM element in the main scope because you look it up in both of your functions.
 var $public = $('#public');
 /* use "var" or publicFormValidation becomes a wild global */
 var publicFormValidation = function(){
 // Cache DOM elements that you're going to call multiple times, like ".add_many_fields"
 // DOM lookups are very expensive operations (depending on the size of the DOM)
 var $addManyFields = $('.add_many_fields'),
 $fileUpload = $('.field_type option[value="file_upload"]'),
 $contacts = $('.field_type option[value="contacts"]');
 $public.change(function(){
 if(checkForm()){
 $fileUpload.hide();
 $contacts.hide();
 $addManyFields.on('click',function(){
 setTimeout(function(){
 $fileUpload.hide();
 $contacts.hide();
 }, 500);
 }); 
 }else {
 $contacts.show();
 $file_uploads.show();
 $addManyFields.on('click',function(){
 setTimeout(function(){
 /* You definitely don't want to do this:
 var $contacts = $('.field_type option[value="contacts"]');
 var $file_uploads = $('.field_type option[value="file_upload"]');
 you're actually overwriting variables you've already defined in the parent scope - you 
 already have these references, so no need to do another DOM lookup.
 */
 $contacts.show();
 $file_uploads.show();
 }, 500);
 });
 };
 });
 }
 /* Use var here as well to get rid of globals */
 var checkForm = function(){
 var public_val = $public.val();
 return (public_val == "true");
 }
}());