Getters and Setters for localStorage

Question

I have written the following get/set for a username which is stored in local Storage. Do you think this "buys" me anything or even perhaps has major disadvantages? I have never liked relying on global variables so I figured the getter and setter would be tidier and more maintainable.

var namespace = {};

namespace.username = {
        key: "Username",
        get: function() { return localStorage.getItem(this.key)},
        set: function(value) {localStorage.setItem(this.key, value)}
    };

before the code had "constants" (I know they arent real in JS)

LOCAL_STORAGE = {
    USERNAME: "Username"
}

and everywhere I need access to the username I would write:

localStorage.getItem(LOCAL_STORAGE.USERNAME);

Answer 1

I think this is a matter of taste, I don't see any major disadvantages but no major advantages either. In these cases I would err on the side of simplicity and go with querying localStorage directly.

Now to fully whet my morning CR.SE appetite let's get really nitpicky:

• Pros

  • It will make it marginally easier to change your code if you some day decide to use a different storage mechanism (for example a fallback mechanism if localStorage doesn't exist). This is only marginally easier unless your code is partitioned in such a way that a global search for LOCAL_STORAGE.USERNAME won't work
  • From the standpoint of coding practice, it IS generally considered better to hide all your external dependencies, and a good argument can be made that localStorage is one
  • Gets rid of your LOCAL_STORAGE keys object
  • Getters and setters are familiar to people coming from a java background

• Cons

  • You actually have more global objects now, as window.namespace.username and all it's children are now effectively global (though namespaced)
  • If someone picks up your code they might not see the localStorage references and assume that you don't depend on it. They might therefore make decisions (for example to deploy to an old IE) that might damage stability. (this is the counterpoint to abstracting away external dependencies).
  • Getters and setters are icky and anti-intuitive for someone not coming from the java world
  • Someone has to learn your api to use it the way you want it to be used. While your code makes the assumption that they are accessing username through your object, someone could very reasonably pick up the code, open up devtools, see that there is a Username key in localStorage, and start querying it directly thereby breaking your expectation. In short, it's so easy to access localstorage, there's no reason for them to expect that you abstracted it.
  • Are you going to follow the same pattern for other objects too? You really should pick one pattern and stick to it and only make exceptions when there is a good reason to.
  • Because you're using this someone can use new, or call, or apply and break your username functions

• General Notes

  • localStorage.getItem('Username') is in no way worse than localStorage.getItem(LOCAL_STORAGE.USERNAME). I would wager it's better because it is less typing and more obvious. Butbutbut it's a magic string! This is javascript, not java, it's all magic strings. Note that my own personal preference is to use ' to differentiate a token (similar to ruby's :token) from text for which I use ".
  • LOCAL_STORAGE - I really dislike that naming convention. Whatever works for you I guess, but what is the semantic meaning of all-caps? How is it semantically different than namespace.localStorageKeys?
  • Do you really need a setter? I image that you're only going to be setting Username in one place - why make a global method then?

  • So you still want to abstract away localStorage? How about a more javascript-y approach?

    myNamespace.storage = function(key, val) {
      return val === undefined ? 
      localStorage.getItem(key) : localStorage.setItem(key, val);
    }
    

    then:

    myNamespace.storage('Username', "George");
    myNamespace.storage('Username') == "George";
    

    This is of course a very naive implementation, you could probably add some more logic handling serializing/deserializing objects from json and fallback mechanisms.

Answer 2

George Mauer already provided a wonderful answer I totally agree with him. I have only one focus in this review and that is speed.

---

Your code is very slow/not very efficient right now, even though it may not totally seem like it. This is because you are interacting with localStorage too much, and interacting with localStorage can be slow (I mean, you are accessing this external memory bank for data perhaps many times).

An easy way to speed this up would be to store the local storage in this object itself. To do this, you could create an init method that simply copies local storage into the object:

init: function() {
    for(var key in localStorage) {
        if(localStorage.hasOwnProperty(key)) {
            this.storage[key] = localStorage[key];
        }
    }
}

Then, when called, will go through local storage, taking all of it's data and copying it into this storage object (you can call it something else). Now, instead of accessing localStorage when looking for and setting data, we can use this.storage:

get: function(key) { return this.storage[key]; }
set: function(key, val) { this.storage[key] = val; }

This code will be much faster now because rather than constantly accessing local storage, you can access this local object instead.

Of course, this data will not be stored when the browser closes, so another method is needed to save this data back to local storage. You can call this save:

save: function() {
    ... copy this.storage data into localStorage ...
}

---

As was pointed out to me, this could potentially slow the heck out of a program when first initialized if the storage contains a lot of data.

As a simple fix, you can have have the init method take an optional parameter of which data to take over in the form of keys:

init: function(keys) {
    ...
    // if keys was supplied and this current key is in the list of keys to take
    if(keys !== undefined && keys.indexOf(key) !== -1) {
        this.storage[key] = localStorage[key];
    }
}

This should be good enough. However, if you want to go a step further, you can also have another optional parameter for specifying how much data to read in. Ex: 1500 bytes.

This would be much more complicated. For every piece of data you read in, you would have to read how big that data is and add it to a total data size counter. Then, once you read too much, you'll have to trim down data from the last read you just did.

I don't have time to cook up an example now, but you might be able to come up with this on your own.