6
\$\begingroup\$

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;
    }
\$\endgroup\$
1
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ "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. \$\endgroup\$ – Pavlo Jan 27 '15 at 20:43
  • \$\begingroup\$ I meant to make it a self invoking function, I'll update it. \$\endgroup\$ – pllee Jan 27 '15 at 20:53
  • \$\begingroup\$ 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). \$\endgroup\$ – pllee Jan 27 '15 at 21:00
  • 1
    \$\begingroup\$ ++ operator was intended to be used like so, while you are abusing typecasting with + operator. \$\endgroup\$ – Pavlo Jan 27 '15 at 21:24
1
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ 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. \$\endgroup\$ – jmoreno Jan 26 '15 at 21:46
  • 1
    \$\begingroup\$ @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. \$\endgroup\$ – pllee Jan 27 '15 at 2:37
  • \$\begingroup\$ @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. \$\endgroup\$ – jmoreno Jan 27 '15 at 2:54
  • \$\begingroup\$ Plavo, you modify the prototype outside of the constructor, what about doing it inside? I like to keep code as self contained as possible. \$\endgroup\$ – jmoreno Jan 27 '15 at 2:57
  • 1
    \$\begingroup\$ @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. \$\endgroup\$ – pllee Jan 27 '15 at 3:09

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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