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've been taking a stab at implementing test cases using the Jasmine testing framework. To do so, I've made an application which has a User object. This User object is created by the server, but its ID is stored in a couple of client-side data storage locations. When a new User model is initialized I check those storage locations before fetching User data from the server.

My model is doing too much work in the constructor. This is making it difficult to test. I am wondering how I could improve my architecture to make it easier to test and also if there are any other mistakes with how I am implementing Backbone, Require, and Jasmine.

//  A singleton representing the sole logged on user for the program.
//  Tries to load itself by ID stored in localStorage and then by chrome.storage.sync.
//  If still unloaded, tells the server to create a new user and assumes that identiy.
define(['programState'], function(programState) {
    'use strict';
    var userIdKey = 'UserId';

    //  Loads user data by ID from the server, writes the ID
    //  to client-side storage locations for future loading and then announces
    //  that the user has been loaded fully.

    function fetchUser(shouldSetSyncStorage) {
        this.fetch({
            success: function(model) {
                //  TODO: Error handling for writing to sync too much.
                //  Write to sync as little as possible because it has restricted read/write limits per hour.
                if (shouldSetSyncStorage) {
                    chrome.storage.sync.set({ userIdKey: model.get('id') });
                }

                localStorage.setItem(userIdKey, model.get('id'));

                //  Announce that user has loaded so managers can use it to fetch data.
                model.trigger('loaded');
            },
            error: function (error) {
                console.error(error);
            }
        });
    }

    //  User data will be loaded either from cache or server.
    var User = Backbone.Model.extend({
        defaults: {
            id: localStorage.getItem(userIdKey),
            name: '' 
        },

        urlRoot: programState.getBaseUrl() + 'User/',

        //  TODO: I am doing too much work in this initialize constructor. 
        initialize: function () {
            //  If user's ID wasn't found in local storage, check sync because its a pc-shareable location, but doesn't work synchronously.
            if (this.isNew()) {
                var self = this;
                //  chrome.Storage.sync is cross-computer syncing with restricted read/write amounts.

                chrome.storage.sync.get(userIdKey, function (data) {
                    //  Look for a user id in sync, it might be undefined though.
                    var foundUserId = data[userIdKey];

                    if (typeof foundUserId === 'undefined') {

                        //  No stored ID found at any client storage spot. Create a new user and use the returned user object.
                        self.save({}, {

                            //  TODO: I might need to pull properties out of returned server data and manually push into model.
                            //  Currently only care about userId, name can't be updated.
                            success: function (model) {
                                //  Announce that user has loaded so managers can use it to fetch data.
                                self.trigger('loaded');
                            },
                            error: function(error) {
                                console.error(error);
                            }
                        });

                    } else {
                        //  Update the model's id to proper value and call fetch to retrieve all data from server.
                        self.set('id', foundUserId);
                        fetchUser.call(self, false);
                    }
                });

            } else {
                //  User's ID was found in localStorage. Load immediately.
                fetchUser.call(this, true);
            }
        }
    });

    //  Return an already instantiated User model so that we have only a single instance available.
    return new User();
});

and here is my sole test case currently with notes on the others:

//  Test cases for the background's user model. Hopes to ensure that the user
//  loads successfully from the server from a client-side id or, alternatively,
//  that it is created successfully by the server.
define(['user'], function (user) {
    'use strict';
    var userIdKey = 'UserId';

    describe('The User', function () {
        //  TODO: The way user is currently written isn't very testable.
        //  I would like to be able to test more specific actions such as the tests left blank here.

        xit('creates when no id found in storage locations', function () {
            localStorage.setItem(userIdKey, null);
        });

        xit('loads from chrome.sync if no id found in localStorage', function () {

        });

        xit('loads from localStorage', function() {

        });

        //  Makes sure a user loads. A bad test case because the user's loadability is dependent on code
        //  that isn't modifiable by this method, so I can only infer current state.
        it('loads', function () {
            var userLoaded = false;
            runs(function () {
                user.on('loaded', function () {
                    userLoaded = true;
                });
            });

            waitsFor(function() {
                return userLoaded === true;
            }, "The user should have loaded.", 5000);

            runs(function() {
                expect(user.isNew()).toBe(false);
            });
        });

    });
});
share|improve this question

1 Answer 1

From a once over:

  • The trigger name 'loaded' should be a constant, declared on the userIdKey level
  • You should not be using console.log, at the very least you should consoledate those calls into a function.
  • It seems that if the user needs to be created thru .save(), that you are not storing the userIdKey neither in localStorage nor chrome.storage, rendering most of the code meaningless? I could be wrong.
  • It also seems that if you find the localStorage item, you force the writing to chrome.storage, why ? If it's in the localStorage, I would expect it in the chrome.storage as well, at the very least you should try a get before you try a set to reduce writing to chrome.storage ?
  • fetchUser should be part of the model, then you dont have to use .call()
  • JsHint could find nothing to complain about
  • Commenting might be a tad excessive
  • I can easily understand what the code does (that is, if my prior analysis is correct)

I don't think that too much is going in initialize, but I do think that you need to work some more on this code.

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.