2
\$\begingroup\$

I've got an Angular controller where I have two functions that are repeated inside two functions:

(function () {

    'use strict';

    angular
        .module( 'app.purchases.products' )
        .controller( 'ReadProductController', ReadProductController );

    ReadProductController.$inject = [ '$scope', 'ReadProductFactory' ];

    function ReadProductController( $scope, ReadProductFactory ) {

        /* jshint validthis: true */
        var vm = this;

        vm.products          = {};
        vm.product           = {};
        vm.getProductsList   = getProductsList;
        vm.getProductDetails = getProductDetails;

        function getProductsList( columnOrder, sortOrder ) {

            var data = {
                columnOrder: columnOrder,
                sortOrder: sortOrder
            };

            ReadProductFactory.listProducts( data, success, fail );

            //The following are the callbacks funcs but they repeat 
            //for the getProductDetails func too, should i set them global?
            function success( products ) {
                vm.products = products;
            }

            function fail( error ) {
                console.log( error );
            }
        }

        function getProductDetails( id ) {

            var data = {
                id: id
            };

            ReadProductFactory.detailProduct( data, success, fail );

            function success( product ) {
                vm.product = product;
            }

            function fail( error ) {
                console.log( error );
            }

        }

    }

})();

The above is the controller of a Product Listing view, i've tried to take rest logic to a factory, what i feel is that my code is not completely DRY because of the callback functions, maybe you'll understand better what i've tried to do with the factory code, here it is:

(function () {

    'use strict';

    angular
        .module( 'app.purchases.products' )
        .factory( 'ReadProductFactory', ReadProductFactory );

    ReadProductFactory.$inject = [ 'Restangular' ];

    function ReadProductFactory( Restangular ) {

        return {
            detailProduct: detailProduct,
            listProducts: listProducts
        };

        function detailProduct( data, success, fail ) {

            Restangular
                .one( '/purchases/products/', data.id )
                .all( '/detail' )
                .getList()
                .then( success, fail );

        }

        function listProducts( data, success, fail ) {


            Restangular
                .all( '/purchases/products/list' )
                .getList()
                .then( success, fail );

        }

    }

})();

Somes have told me to use promises instead of callback, how could i do that? Also, is it good practice to declare empty arrays as I've done?

\$\endgroup\$
1
\$\begingroup\$

First, your controller is written weirdly. It could be simplified into:

// Since we're inlining everything, we don't need to wrap it in IIFE.
// Also, `readProductController` isn't a good name. Controllers are things,
// nouns, not verbs.
angular.module( 'app.purchases.products' ).controller('ProductController',[

  // A controller accepts either a function with implicit dependencies based on
  // argument names, or an array of dependency names, and whose last element
  // is the controller function.
  '$scope', 'productFactory'
, function(

  // I've read somewhere that factories and services should be named with first
  // letter as small since we're not exporting a constructor. Reserve Capital
  // names for things that return a constructor function.
  $scope, productFactory
){

  // Not sure why `vm` exactly. It doesn't tell me what it is. Name it something
  // meaningful, like `controller`.
  var controller = this;
  controller.products = {};
  controller.product = {};

  // You can simply inline the functions. No need for the indirection.
  this.getProductsList = function(columnOrder, sortOrder){

    // Promises are simply just objects that hold state for an async operation.
    // They still accept "callbacks" via the `then` method they provide.
    productFactory.getProducts({
      columnOrder: columnOrder,
      sortOrder: sortOrder
    }).then(function(products){
      controller.products = products;
    }, function(error){
      console.log(error);
    });
  };

  this.getProductDetails = function(id){
    productFactory.getProductDetails({
      id: id
    }).then(function(product){
      controller.product = product;
    }, function(error){
      console.log(error);
    });
  };


}]);

We can do the same with your service

// `readProductFactory` isn't really a good factory name. Factories are things
// that create things. They're not actions, so drop the read. It's just
// productFactory starting now.
angular.module('app.purchases.products').factory('productFactory', [
  'Restangular',
function(
  Restangular
){
  return {

    // Functions are "actions", verbs, commands. They should be named in a
    // "do something to that something" manner, like `getList`, or `sendMail`
    getProducts: function(data){

      // Restangular appears to already return a promise (because you "then'ed")
      // All we have to do is just return that promise to the controller so that
      // it listens with `then`.
      return Restangular.all( '/purchases/products/list' ).getList()
    },
    getProductDetails: function(data){

      // Doing the same
      return Restangular.one( '/purchases/products/', data.id ).all( '/detail' ).getList()

    }
  };
}]);
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ The "Wierd" code is the code style introduced by John Papa, vm stands for View-Model, the reason why i named the controller ReadProductsController it's because i'm trying to keep the singleton concept, so i decided to create a single Create, Read, Update, Delete, since they all are attached to different Views, but maybe i could create several services and keep one single ProductController, thanks Joseph \$\endgroup\$ – Jonathan Solorzano Nov 8 '15 at 22:11

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.