1
\$\begingroup\$

How can I refactor this code to be more clean?

  $scope.unreserve = function () {
    var params = {
      'wishlist_product_id' : product.wishlist_product_id
    };

    WishlistService.unreserve(params, function (data) {
      if (data.success) {
        $rootScope.$broadcast('NotificationController.create', {type: 'success', message: 'Unreserve successfully'});
      } else  {
        $rootScope.$broadcast('NotificationController.create', {type: 'error', message: data.error_message});
      }
    });
  };

  $scope.createManualProduct = function () {
    var params = {};

    ProductService.addManualProduct(params, function (data) {
      if (data.success) {
        $rootScope.$broadcast('NotificationController.create', {type: 'success', message: 'Unreserve successfully'});
      } else{
        $rootScope.$broadcast('NotificationController.create', {type: 'error', message: data.error_message});
      }
    });
  };
\$\endgroup\$

1 Answer 1

2
\$\begingroup\$

There are 2 main changes I would make:

  1. ProductService.addManualProduct seems to require a callback. A way that is more consistent with Angular, and with a number of benefits, would be to return a promise, so you can use it like:

    ProductService.addManualProduct(params).then(function() {
      // Do something
    });  
    
  2. Using $rootScope should be avoided if there is an alternative, as it's effectively global over the entire app. One way, would be to change the $rootScope.$broadcast to $scope.emit, which can be "picked up" by a directive on some parent element. So you could have a template like:

    <div notification-center>
      <div ng-controller="myController">
         <!-- Buttons here to create/unreserve -->
      </div>
    </div>
    

    And in myController, $emit an event:

    $scope.$emit('notificationCenter::create', someData);
    

    Then in a notificationCenter directive's controller, you can listen to this

    {
       controller: function($scope) {
         $scope.$on('notificationCenter::create', function(e, data) {
           // Do something, like display the message
         });
       }
    }
    

    An alternative approach would be to have the notificationCenter directive inside the template controlled by myController, and then use $scope.$broadcast to send messages to it.

    The "clean" aspect of this is that you could potentially have separate areas of the app, with separate notification areas, if you wish.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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