3
\$\begingroup\$

I have a service which needs to store some settings data in a DB and at the same time maintain a reference to that in the user'slocalStorage (we want two users logged in to the same account to be able to view them differently). This method looks for an existing previewSession in the localStorage and if not found, creates one and generates a unique URL.

As you can see at the beginning, I'm using the $q service to return a promise and then resolving that promise as and when all operations have finished. There is one use of the $http service and the save call is on a $resource object.

Is this a proper way to handle such a use-case, or could I have done it better?

To summarize the requirements:

  1. Method gets called with a settings parameter
  2. Method must always return a URL (unless an error occurs on the db level or some such thing)

var PREVIEW_SESSION_KEY_BASE = '<a constant string>';    
function getPreviewSessionUrl(settings) {
        var deferred = $q.defer();

        $http.get('/' + appCode + '/reservedDomain',
            function(appResponse) {
                $localForage
                    .getItem(PREVIEW_SESSION_KEY_BASE + themeId)
                    .then(function(data) {
                        if (data) {
                            deferred.resolve(appResponse + '?<a query key>=' + data);
                        } else {
                            previewSessionResource.save(
                                {},
                                {
                                    settings: settings
                                },
                                function(previewResponse) {
                                    $localForage
                                        .setItem(PREVIEW_SESSION_KEY_BASE + themeId, previewResponse)
                                        .then(function(data) {
                                            deferred.resolve(appResponse + '?<a query key>=' + data);
                                        });
                                },
                                function(error) {
                                    deferred.reject(error);
                                }
                            );
                        }
                    });
            },
            function(error) {
                deferred.reject(error);
            }
        );

        return deferred.promise;
    }
\$\endgroup\$
  • \$\begingroup\$ Not much of a review, but simply the fact that you have a pyramid of doom style code, implies that you're using promises wrong. \$\endgroup\$ – Madara Uchiha Dec 3 '14 at 10:59
  • \$\begingroup\$ True. I completely disregarded chaining of the then calls. Thank you! \$\endgroup\$ – ScionOfBytes Jan 14 '15 at 5:33
1
\$\begingroup\$

Things that could have been better;

  • This:

            },    
            function(error) {
                deferred.reject(error);
            }
        );
    

    could simply be

            }, deferred.reject );   
    

    reject is a function that takes error as a parameter after all..

  • I would have externalized the previewSessionResource.save(call, it makes the reading too hard. You only capture settings and appResponse which you would then provide as parameters.
  • If you control the signature of previewSessionResource.save, then I would simply request for settings as an object instead of requesting for { settings: settings } to keep it simple, it does not make sense to wrap a single data point into an object.
  • I am also idly wondering if previewSessionResource.save should not be responsible for calling $localForage.setItem(), would you ever call previewSessionResource.save without calling $localForage.setItem()?

On the whole, I think this is okay, it just needs some rejiggering to read better.

\$\endgroup\$
  • \$\begingroup\$ Thanks @konijn! Yeah, readability is the main issue here. I'll refactor the code, accordingly. \$\endgroup\$ – ScionOfBytes Jan 14 '15 at 5:31

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.