[Code Review] Check Javascript coding practice and overall performance

JavaScript
1

I have a Edit/Details form which has 4 user related fields. On click of Save, I save the edited fields to local storage (if supported) and display the same values in the Details view.

Below is the code which does that;


      var UserDataObj = {
    name:"John",
    email:"john@example.com",
    phone:"9999999999",
    desc:"some description"
    }

    /**
     * Check HTML5 Local Storage support
     */
    function supports_html5_storage()
    {
        try {
            window.localStorage.setItem( 'checkLocalStorage', true );
            return true;
        } catch ( error ) {
            return false;
        };
    };

    /**
     * Save form data to local storage
     */
    function saveFormData()
    {
    	if (supports_html5_storage())
    	{
    	$(".formFieldUserData").each(function(){
    		UserDataObj[$(this).attr("name")] = $(this).val();
    	});
    	
    	localStorage.setItem('UserDataObj', JSON.stringify(UserDataObj));	
    	}
    }


    /**
     * Set field values based on local storage
     */
    function setFormFieldValues()
    {
    	if (supports_html5_storage())
    	{
    			var retrievedUserDataObj = JSON.parse(localStorage.getItem('UserDataObj'));
    			if(retrievedUserDataObj)
    			{
    				$(".formFieldUserData").each(function(){
    					var currentKey = $(this).attr("name");
    					var retrievedValue = retrievedUserDataObj[$(this).attr("name")]
    					$("#"+currentKey).val(retrievedValue);
    					$("#"+currentKey+"Txt").html(retrievedValue);
    				});
    			}
    			else
    			{
    				$(".formFieldUserData").each(function(){
    					var currentKey = $(this).attr("name");
    					var userDataObjValue = UserDataObj[$(this).attr("name")]
    					$("#"+currentKey).val(userDataObjValue);
    					$("#"+currentKey+"Txt").html(userDataObjValue);
    				});
    			}
    	}
    	else
    	{
    		$(".formFieldUserData").each(function(){
    		var currentKey = $(this).attr("name");
    			if ($(this).val() != "")
    			{
    			var fieldValue = $(this).val();
    			$("#"+currentKey).val(fieldValue);
    			$("#"+currentKey+"Txt").html(fieldValue);
    			}	
    			else
    			{
    			var defaultuserDataObjValue = UserDataObj[$(this).attr("name")];
    			$("#"+currentKey).val(defaultuserDataObjValue);
    			$("#"+currentKey+"Txt").html(defaultuserDataObjValue);
    			}
    		})
    	}
    }

My question is;

  1. This is a completely working code. But is there any further that I can do as far as best coding practices go.
  2. Also performance wise, can I do anything to improve the overall code ?

Thank you.

2

Best practice is to declare all local variables at the top of the function (where the declarations actually take place) rather than scattered through the code.

If you wrap your entire script inside an anonymous function then it will remove the possibility of clashes of field names with other scripts.

Specifying “use strict”; at the top of a function will enforce the new JavaScript rules in those browsers that support it and make it easier to identify some common errors.

3

A few stylistic things you should change are:

consistent casing: rename supports_html5_storage to supportsLocalStorage.
For feature detection scripts like these I’d recommend looking at http://modernizr.github.com/Modernizr/annotatedsource.html you’ll learn a lot from their comments.


function supportsLocalStorage() {
  try {
      return !!localStorage.getItem;
  } catch(e) {
      return false;
  }
};

curly braces on the same line as conditions / function declarations e.g.
function yep() {
alert(‘Crockford says so’);
}

But the biggest code smell in your example is the big fork and duplication in setFormFieldValues.
The two forks don’t do the same thing, if you need to persist the userData across page refreshes you need to use cookies/localstorage, if you don’t then just use the hash. Pick one or the other, don’t use both methods because they are not the same.

If you do want to use localstorage then use a polyfill so that you don’t need to fork the code for the implementation differences. e.g. https://gist.github.com/remy/350433
Sure, it uses a fair bit of code - but this is preferable to differing implementations.

If you take away those differences the script becomes something like this.


var userData = {
  name:  "John",
  email: "john@example.com",
  phone: "9999999999",
  desc:  "some description"
}

function saveData() {
  $(".formFieldUserData").each(function(){
    userData[$(this).attr("name")] = $(this).val();
  });
  localStorage['userData'] = JSON.stringify(userData);
}

function loadData() {
  userData = JSON.parse(localStorage['userData']);
  $(".formFieldUserData").each(function() {
    var key = $(this).attr("name");
    var value = userData[$(this).attr("name")]
    $("#"+key).val(value);
    $("#"+key+"Txt").html(value);
  });
}