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.

Looking for general feedback on this module. remember is property that a user sets via clicking "remember me" when logging in.

A token identifies the user - h_token. If localStorage.h_token is set, it is assumed the user has clicked remember me, this is checked at load time.

When the remember state is changed manually by the user setRemember() is called.

Not looking for advice on any security issues or naming conventions at the moment it is not a concern.

Just general things.

/*SClientStorage
** 
** 
**
*/
SStorage = NS.support({
    Name: 'Storage',
    hold: {},
    clientStorage: localStorage.h_token ? localStorage : sessionStorage,
    setRemember: function (remember) {
        this.clientStorage = remember ? localStorage : sessionStorage
    },
    clear: function () {
        this.clientStorage.clear();
    },
    setAll: function (o) {
        var key;
        for (key in o) {
            if (o.hasOwnProperty(key)) {
                this.clientStorage[key] = o[key];
            }
        }
    },
    getAll: function () {
        var o = {},
            key;
        for (key in this.clientStorage) {
            if (this.clientStorage.hasOwnProperty(key)) {
                o[key] = this.clientStorage[key];
            }
        }
        return o;
    },
    set: function (key, value) {
        this.clientStorage[key] = value;
    },
    get: function (key) {
        return this.clientStorage[key];
    }
});
share|improve this question
add comment

2 Answers 2

up vote 2 down vote accepted

Looks really good. It's clear and easy to understand. Regarding this specific implementation, a couple of things that came to mind:

  • In setRemember(), unless you specifically only want to support the values 1 or 0, the code can be changed to:

    • this.clientStorage = remember ? this.localStorage : this.sessionStorage;
  • In setRemember(), when switching storage mechanisms it might make sense to copy the values out of the old mechanism into the new one so anything the user has previously set will be preserved. Similary, if the user is changing from localStorage to sessionSorage, would it make sense to also clear the old storage mechanism? If making either of these changes, the method could also first compare the new value against the old value and only continue if there is actually a change.

  • What is o used for in log() method?

  • In the log() method, depending on on the utility you're using for logging, you might consider logging the entire object, at once, as many console / logging utilities will allow you to inspect objects. This can sometimes be more informative, although here it's just about whatever would be more convenient because all values will be strings, anyway. If you go this route, you could rename the current function to something like logEach().

  • In the execution of clear(), should you preseve the h_token key?

Taking a step back, here is an alternate API for the utilitiy. Just thinking out loud.

  • ssn(key) - Retrieves a value. Synonymous with get()
  • ssn(key, value) - Sets a value. Synonymous with set()
  • ssn(obj) - Set all values from obj. Synonymous with setAll()
  • ssn.clear() - Unchanged.
  • ssn.clone() - Synonymous with getAll()
  • ssn.log() - Log the clientStorage object as a whole, as described above.
  • ssn.logEach() - Log each property and corresponding value, one at a time.
  • ssn.remember() - Retrieves the current remember setting.
  • ssn.remember(value) - Sets the remember value. Synonymous with setRemember()

One final thought you may want to implement some sort of 'namespace' type of functionality to be able to differentiate between settings saved for different users. I don't suggest this for security reasons, but more from a user-experience perspective. I think, with the way it's set up, multiple users on the same computer will have their settings merged, which may get confusing.

share|improve this answer
    
Session. Although, in a non-technical context I think it can also refer to a social security number. –  tiffon Jan 6 '13 at 11:39
    
I only allow the user to change remember me at login so there is no need to save the previous. Similarly I only use clear() when logging the user out, so I don't need to preserve anything. –  pure_code Jun 4 '13 at 21:07
add comment

replace

if(remember === '1') {
    this.clientStorage = localStorage;
} else if(remember === '0') {
    this.clientStorage = sessionStorage;
}

with

this.clientStorage = [localStorage, sessionStorage][parseInt(remember)]

note: tiffon's

this.clientStorage = remember ? this.localStorage : this.sessionStorage

works very well, is nice and concise, and there is no confusion what is going on. However, if you have to add another option, e.g., '2', '3', '4' then my code suits better.

replace

var key;
for (key in o) {

with

for (var key in o) {
share|improve this answer
    
odd. works in chrome. As this is not universally accepted by all JS then lets not do this. –  rikAtee Jan 4 '13 at 10:49
add comment

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.