SitePoint Sponsor

User Tag List

Results 1 to 3 of 3
  1. #1
    SitePoint Member
    Join Date
    Mar 2013
    Posts
    1
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)

    [Code Review] Check Javascript coding practice and overall performance

    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;

    HTML Code:
          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. #2
    Programming Since 1978 silver trophybronze trophy felgall's Avatar
    Join Date
    Sep 2005
    Location
    Sydney, NSW, Australia
    Posts
    16,826
    Mentioned
    25 Post(s)
    Tagged
    1 Thread(s)
    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.
    Stephen J Chapman

    javascriptexample.net, Book Reviews, follow me on Twitter
    HTML Help, CSS Help, JavaScript Help, PHP/mySQL Help, blog
    <input name="html5" type="text" required pattern="^$">

  3. #3
    padawan silver trophybronze trophy markbrown4's Avatar
    Join Date
    Jul 2006
    Location
    Victoria, Australia
    Posts
    4,108
    Mentioned
    28 Post(s)
    Tagged
    2 Thread(s)
    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/Moderniz...tedsource.html you'll learn a lot from their comments.
    Code javascript:
    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.
    Code javascript:
    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);
      });
    }


Tags for this Thread

Bookmarks

Posting Permissions

  • You may not post new threads
  • You may not post replies
  • You may not post attachments
  • You may not edit your posts
  •