2
\$\begingroup\$

I have a controller that pulls gets data from two different JSON files. One of the calls loadProducts happens when the controller is initiated. The other one, loadPrices, is called when a user clicks a button on the interface.

I have noticed that the controller is starting to get bigger and I was worried about the state of the code. As I am knew to this I was hoping for suggestions about how I can clean it up or make it more efficient.

'use strict';
(function () {

var app = angular.module('priceQuoteApp');

var quoteBuilderController = function ($scope, $http, $timeout) {

    //set the variabls
    $scope.listOfProducts = null;
    $scope.selectedProduct = null;
    $scope.listOfProductVariants = null;
    $scope.formData = {};
    $scope.showTable = 'none';
    $scope.selectedProductId = null;
    $scope.loadingProducts = true;
    $scope.loadedProducts = false;
    $scope.loadingPrices = false;
    $scope.loadedPrices = false;

    // get all the portfolios and the products
    // the timer is to simulate network latency
    var loadProducts = function () {
      $http.get('scripts/json/sample-products.json')
        //$http.get('http://www.thesite/api/Pricing/GetProductCatalogue')
            .then(function (allProducts) {
                $scope.listOfProducts = allProducts.data.Connectivity;
            })
            .finally(function () {
                $scope.loadingProducts = false;
                $scope.loadedProducts = true;
            });
    };
    //loadProducts();
   $timeout(loadProducts, 2000);

    // get all the prices
    // the timer is to simulate network latency
    var loadPrices = function () {
        $http.get('scripts/json/sample-product-prices.json')
            .then(function (res) {
                $scope.selectedProductPrices = res.data.ProductVariants;
                $scope.selectedProductAddOns = res.data.product_addons;
            })
            .finally(function () {
                $scope.loadingPrices = false;
                $scope.loadedPrices = true;
            });
    };

    // get the product details
    $scope.getProductDetails = function (item) {
        $scope.selectedProduct = item.product_name;
        $scope.selectedProductId = item.product_id;
        $scope.listOfProductVariants = item.product_variants;
        $scope.formFields = item.product_price_attributes;
    };

    // select the product and get the price
    $scope.selectProduct = function (item) {
        $scope.selectedRow = item;
        $scope.selectedProductPrice = item.RecurringCost;
    };

    // replace this with api calls to get the prices
    $scope.getProductVariants = function () {
        $scope.loadingPrices = true;
        $timeout(loadPrices, 2000);
    };
};

app.controller('quoteBuilderController', ['$scope', '$http', '$timeout', quoteBuilderController]);

}());

One this for sure is that I should probably break it up as its becoming long. Is there a need to define the variables at the top? Should I be using the This keyword?

\$\endgroup\$
5
  • 1
    \$\begingroup\$ Use services/factories to encapsulate your $http service calls. Also, there is no need to break up your object model when assigning to a property on $scope. And try to add more than one ngController, that one controller is doing too much. Instead of doing things like "loading=true" - which I assume is for displaying a wait dialog or spinner, consider implementing an $http loading directive, outside of your very large controller \$\endgroup\$
    – pixelbits
    Feb 3, 2015 at 8:31
  • \$\begingroup\$ Sorry Jurge - I didnt know that existed. Pixelbits - many thanks for your detailed answer. I will implement some of that. \$\endgroup\$ Feb 3, 2015 at 8:44
  • 3
    \$\begingroup\$ But you posted a question on Code Review over a year ago! \$\endgroup\$ Feb 3, 2015 at 9:01
  • \$\begingroup\$ Seems you are right 200. I forgot about that. Thanks for looking into and clarifying that for us. \$\endgroup\$ Feb 3, 2015 at 10:07
  • \$\begingroup\$ Hey Pixelbits - what do you mean by 'there is no need to break up your object model when assigning to a property on $scope'? thank you \$\endgroup\$ Feb 4, 2015 at 15:38

2 Answers 2

2
\$\begingroup\$

There is no need using the 'this' keyword, or such as angularjs provides for those scenario. One thing you can do is to abstract the loadProducts functions and load prices function to a factory and then pass the factory as a parameter to your controller as shown below.

var priceQuoteApp = angular.module('priceQuoteApp', [])
    .constant("loadProductUrl", "scripts/json/sample-products.json")
    .constant("loadPricesUrl", 'scripts/json/sample-product-prices.json');

priceQuoteApp.factory("ProductsFactory", function ($http, loadProductUrl, loadPricesUrl) {
        return {
            loadProducts: function () {
                //Create an object to hold your result
                var result = {};

                $http.get(loadProductUrl)  
                .then(function (allProducts) {
                    result.listOfProducts = allProducts.data.Connectivity;
                })
                    .finally(function () {
                        result.loadingProducts = false;
                        result.loadedProducts = true;
                    });
            },

            loadPrices: function () {
                $http.get(loadPricesUrl)
                    .then(function (res) {
                        result.selectedProductPrices = res.data.ProductVariants;
                      result.selectedProductAddOns = res.data.product_addons;
                    })
                    .finally(function () {
                        result.loadingPrices = false;
                        result.loadedPrices = true;
                    });
            }
        });

Then you inject the factory into your controller as shown below

priceQuoteApp.controller("quoteBuilderController", function ($scope, $http, $timeout, ProductsFactory) {
        var products = ProductsFactory.loadProducts();
        //Your other codes

        //When the user clicks the button
        $scope.clickAction = function () {
            var prices = ProductsFactory.loadPrices;
            //Then do other things with it.
        }
    });

One good thing is that this increases code return and thereby making every part of your code to focus on a single responsibility. Hope this helps.

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

It might help you!!! Here I'm using services and controller in separate parts.

var app = angular.module('priceQuoteApp');

app.service('quoteBuilderServices', function ($http) {
    this.getProducts = function () {
        return $http.get('scripts/json/sample-products.json').
        success(function(resp){
            //log something here.
        });
    };
});

app.controller("quoteBuilderController", function($scope, $timeout, quoteBuilderServices) {

   //Set the variabls
    $scope.listOfProducts = null;
    $scope.loadingProducts = true;
    $scope.loadedProducts = false;

   // Calling here services method.
    quoteBuilderServices.getProducts()
      .then(function (allProducts) {
         $scope.listOfProducts = allProducts.data.Connectivity;
      })
      .finally(function () {
         $scope.loadingProducts = false;
         $scope.loadedProducts = true;
      });       
});
\$\endgroup\$
0

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.