4
\$\begingroup\$

We have the following validation function:

    var result = scope.CurrentUser.Surname.length > 0;
    var d = $q.defer();

    if (result) {
        apiService.UniqueEmailCheck(scope.CurrentUser.PersonId, scope.CurrentUser.Email)
            .success(function (response) {
                if (response) {
                        apiService.GetSelectetExistent(scope.CurrentUser.FirstName, scope.CurrentUser.Surname, scope.CurrentUser.DOB)
                        .success(function (response) {
                                    $scope.SelectExistent = response;
                                    d.resolve($scope.SelectExistent.length > 0);
                        });

            }
            else {
                d.resolve(false);
            }
        })
}
else
    d.resolve(false);

return d.promise;

If the incoming result is true, we have to check one or two async conditions.

It works as expected but we are not sure if we can do better. Any suggestions?

\$\endgroup\$
  • \$\begingroup\$ Hi, welcome to Code Review! I hope you receive great answers! \$\endgroup\$ – Tunaki Apr 12 '16 at 11:25
3
\$\begingroup\$

Consider having your APIs return a promise instead. By the looks of it, success indicates that it's a Q promise which means it should be able to do then. This means we can throw away the "outer deferred" and use the promise directly.

Both requests don't necessarily need to be in sequence. The second API call doesn't seem to require the first one other than the fact that it checks for uniqueness and bails out if not unique. You can fire both in parallel, with a minor tradeoff that it will fire 2 in all cases. But then, 2 in parallel vs 2 in sequence. Time-wise, I'd prefer the former.

If you have native promises available or are able to polyfill it, then use native instead. Also, instead of resolving to a boolean, use the promise state as your boolean. Resolve or reject accordingly.

Your code can then be simplified to:

var result = scope.CurrentUser.Surname.length > 0;

var uniquenessRequest = apiService.UniqueEmailCheck(scope.CurrentUser.PersonId, scope.CurrentUser.Email);

var existenceRequest = apiService.GetSelectetExistent(scope.CurrentUser.FirstName, scope.CurrentUser.Surname, scope.CurrentUser.DOB).then( response => {
  $scope.SelectExistent = response;
  return response;
});

return !result ? Promise.reject() : Promise.all([
  uniquenessRequest,
  existenceRequest
]).then(results => {
  var pass = response[0] && response[1].length > 0;
  return Promise[ pass ? 'resolve' : 'reject' ]();
});
\$\endgroup\$
  • \$\begingroup\$ although I have to modify your code for business constraint, I like your approach, thank you very much! \$\endgroup\$ – alessandro Apr 12 '16 at 17:48

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.