Saving preferences in localStorage

Question

I am working on a small app to help my kids study their multiplication and addition. And I have various preferences I want to be persisted between visits. What do you think of the below to get the settings out of local storage? I know I could do it even more simply by simply stringifying the whole object at once, but I want the preferences to contain the answers and I don't want to save all of the answers every time the preferences change...although as I write this that seems like less of an issue...this is for my kids so I don't care about supporting older browsers.

function getPrefs() {
 "use strict";
 getPrefs.prefs = getPrefs.prefs || new Object();
 var prefs = getPrefs.prefs;
 if (prefs.populated) return prefs;
 var localStorage = window['localStorage'];
 prefs.addAnswer = function (answer, correct) {
 this.answers.unshift(answer);
 if (correct) {
 this.correctAnswers++;
 localStorage.setItem("correctAnswers", this.correctAnswers.toString());
 if (this.correctAnswers % 12 === 0) {
 this.level++;
 localStorage.setItem("level", this.level.toString());
 }
 } else {
 this.wrongAnswers++;
 localStorage.setItem("wrongAnswers", this.wrongAnswers.toString());
 }
 localStorage.setItem("answers", JSON.stringify(this.answers.slice(0, 100)));
 this.answeredAdded();
 }
 prefs.answeredAdded = function () { };
 prefs.answers = JSON.parse(localStorage.getItem("answers") || '[]');
 prefs.topValue = Number(localStorage.getItem("topValue"));
 prefs.bottomValue = Number(localStorage.getItem("bottomValue"));
 prefs.showHistory = localStorage.getItem("showHistory") === "false";
 prefs.operator = localStorage.getItem("operator") || "x";
 prefs.correctAnswers = Number(localStorage.getItem("correctAnswers") || 0);
 prefs.wrongAnswers = Number(localStorage.getItem("wrongAnswers") || 0);
 prefs.level = Number(localStorage.getItem("level") || "1");
 prefs.populated = true;
 return prefs;
 }

Answer 1

Pavlo has a good answer I'll just comment on a few extra things.

You can use a closure to init your prefs object only once. It is a little bit cleaner than putting a flag on it to show that it has been inited. You can do something like:

var getPrefs = (function() {
 var prefs;
 return function() {
 if(prefs) {
 return prefs;
 }
 prefs = ...
 }
})();

The variable prefs will always be falsy the first time around.

Design wise I would just jsonify everything, there isn't much overhead and it makes things simpler without the need to cast everything.

JS coding tidbits:

Use {} over new Object() it is slightly cleaner and the preferred way.

You can use the + operator when casting Numbers +'1' will return the number 1 for example.

• Pavlo: "You can use the + operator when casting Numbers" – please don't. Shorter isn't always better, software engineering learned that lessons the hard way from Perl.
• pllee: I meant to make it a self invoking function, I'll update it.
• pllee: Shorter isn't always better for thinks like variable/function names and for the most part readability but I would argue for this case it is. The plus operator is widely known as a Number cast. I would say this falls under something like i++ vs i += 1 I guess it is a preference but I will always choose i++ (even ignoring the incrementing vs addition aspect).
• Pavlo: ++ operator was intended to be used like so, while you are abusing typecasting with + operator.

Answer 2

Design

Since you want only one instance of preferences, you can make use of the singleton pattern:

var prefs = (function () {
 var instance;
 return {
 getInstance: function () {
 if (!instance) instance = new Prefs();
 return instance;
 }
 };
})();

Then you can call the method wherever needed: prefs.getInstance().

You can also encapsulate initialisation logic into a constructor, it seems like you wanted to use OOP anyway:

function Prefs() {
 // init your properties here, use 'this', e.g:
 this.answers = [];
}
Prefs.prototype.addAnswer = function (answer, correct) {
 // use 'this' just as you did in your code
};

localStorage

The way you deal with localStorage seems ok to me. If you want, you can turn correctAnswers and wrongAnwsers into setters, tough syntax for them is a bit baroque:

Object.defineProperty(Prefs.prototype, 'correctAnswers', {
 set: function (val) {
 localStorage.setItem('correctAnswers', val);
 return val;
 }
});

Use it as you previously did: this.correctAnswers++.

Minor issues

This line is useless:

var localStorage = window['localStorage'];

You can skip toString here:

localStorage.setItem("correctAnswers", this.correctAnswers.toString());

topValue and bottomValue don't have default values. They will be NaN if there is no such item in localStorage.

• jmoreno: I had considered a singleton (one of the reasons I posted), but but couldn't really find a justification for that vs function. The window['localStorage'] being useless, that was the example I've seen, but doesn't it expicitly define the scope so that if localStorage was define elsewhere it wouldn't conflict? Not a problem at the moment, but it seemed a good practice. Asfor top and bottom, I was taking checking for that elsewhere to provide defaults, but this might be a better place to set it.
• pllee: @jmoreno the var localStorage line is basically making a local variable the same name and instance of a global variable. That line would be useful to exit gracefully in older browsers, in your case it really doesn't matter.
• jmoreno: @pllee: yes, I was asking about the value of making a local variable with the same name and instance as a global, in case there was another local variable between my function and the global scope which has the same name but a different instance. Not sure if that is being extra paranoid or not, but I'm looking to do things right.
• jmoreno: Plavo, you modify the prototype outside of the constructor, what about doing it inside? I like to keep code as self contained as possible.
• pllee: @jmoreno it is still basically the same thing. In this case localStorage, window.localStorage and window['localStorage'] are all the exact same thing. Assigning a variable named localStorage to the localStorage object doesn't gain really any safety.