Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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:"[email protected]",
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?
share|improve this question
    
Note that this has been crossposted to Stack Overflow (where it is closed) and Programmers (where I think it is O/T). –  halfer Mar 10 '13 at 11:53

1 Answer 1

  • You can start with how you code. Have them indented properly and checked by a quality-check tool. Code-review doesn't only deal with optimization, but also maintainability of your code and that includes making it readable.

  • After running it in JSHint, I found some minor problems in your code. Most notable are missing semi-colons. Although semi-colons in line endings are optional in JS, it's still best practice to include them. You could also avoid code packer/compressor errors by actually putting them in. This avoids your lines from being connected to the next line.

  • Use strict comparison as much as possible in JS. JS treats "falsy" values as false, which includes null, undefined, 0, false, a blank string and NaN. For example, `"" == false is actually equal.

  • Place your code in a namespace, or some inner-scope to avoid polluting the global scope.

For code related stuff:

(function (ns, $) {
  "use strict"; //prevents "bad practice"

  //variables needed for localStorage test
  var storage, fail, uid;

  //For flexibility, it would be best if your code 
  //wasn't build for a specific form only
  //instead, allow it to create instances and attach
  //to different forms with different default data
  function FormSave(selector, defaultValues) {
    this.defaultValues = defaultValues || {};
    this.dataName = selector;
    this.form = $(selector);
    this.userFields = $('.formFieldUserData', this.form);
  }

  //our save function
  FormSave.prototype.save = function () {

    var userData = {};

    //using the "early return" pattern to avoid further
    //code exectution if condition fails, and avoid
    //deep indentation
    if (!storage) {
      return;
    }

    //since each() assigns the current DOM element in
    //the collection as this, we can get name and value
    //directly from the DOM element, saving you function calls
    this.userFields.each(function () {
      userData[this.name] = this.value;
    });

    //set the items
    localStorage.setItem(this.dataName, JSON.stringify(userData));
  };

  //our setting function
  FormSave.prototype.set = function () {

    //cache the default values into this scope
    var defaultValues = this.defaultObject,
      retrievedValues;

    //if local storage is present, get and parse
    if (storage) {
      retrievedValues = JSON.parse(localStorage.getItem(this.dataName));
    }

    this.userFields.each(function () {

      //cache the current key
      var currentKey = this.name,
        //then the value to be used is:
        //if storage is present, use either the retrieved values or the default
        //if not, then use the present value or the default
        value = ((storage) ? retrievedValues[currentKey]: this.value) || defaultValues[currentKey];

      //for safe text insertion, use text() since it escapes the value first
      $("#" + currentKey + "Txt").text(value);
      $("#" + currentKey).val(value);
    });

    //our easy binding function exposed to our FormSave namespace
    ns.bind = function (selector) {
      return new FormSave(selector);
    };
  };

  //a quick test for localstorage
  //from: http://mathiasbynens.be/notes/localstorage-pattern
  try {
    uid = new Date();
    (storage = window.localStorage).setItem(uid, uid);
    fail = storage.getItem(uid) !== uid;
    storage.removeItem(uid);
    fail && (storage = false);
  } catch (e) {}

}(this.FormSave = this.FormSave || {}, jQuery));

//usage:

//bind this functionality to a form id'ed "someForm" with
//the given default values
var someForm = FormSave.bind('#someForm', {
  name: 'John',
  email: '[email protected]',
  phone: '9999999999',
  desc: 'some description'
});

//then with the given reference, call save and set
someForm.save();
someForm.set();

When packed, it's this small!

(function (ns, $) {
  "use strict";
  var storage, fail, uid;

  function FormSave(selector, defaultValues) {
    this.defaultValues = defaultValues || {};
    this.dataName = selector;
    this.form = $(selector);
    this.userFields = $('.formFieldUserData', this.form)
  }
  FormSave.prototype.save = function () {
    var userData = {};
    if (!storage) {
      return
    }
    this.userFields.each(function () {
      userData[this.name] = this.value
    });
    localStorage.setItem(this.dataName, JSON.stringify(userData))
  };
  FormSave.prototype.set = function () {
    var defaultValues = this.defaultObject,
      retrievedValues;
    if (storage) {
      retrievedValues = JSON.parse(localStorage.getItem(this.dataName))
    }
    this.userFields.each(function () {
      var currentKey = this.name,
        value = ((storage) ? retrievedValues[currentKey] : this.value) || defaultValues[currentKey];
      $("#" + currentKey + "Txt").text(value);
      $("#" + currentKey).val(value)
    });
    ns.bind = function (selector) {
      return new FormSave(selector)
    }
  };
  try {
    uid = new Date();
    (storage = window.localStorage).setItem(uid, uid);
    fail = storage.getItem(uid) !== uid;
    storage.removeItem(uid);
    fail && (storage = false)
  } catch (e) {}
}(this.FormSave = this.FormSave || {}, jQuery));
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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