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.

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});
      }
    });
  };
share|improve this question

1 Answer 1

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.

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.