2
\$\begingroup\$

The following code works, but:

The service makes multiple API calls, the functions are very similar with a lot of code duplication. The same is true for the controllers- very similar code with only slight differences. How could I make this more clean and concise?

app.factory('listDataService', ['$http', function ($http) {
    function firstListData() {
        return $http({
            method: 'GET',
            url: _spPageContextInfo.webAbsoluteUrl + "/_api/web/lists/getbytitle('ListName')/items",
            headers: { "Accept": "application/json;odata=verbose" }
        }).then(function (response) {
            return response.data.d.results;
        }).catch(function (error) {
            console.log("Error: " + JSON.stringify(error));
        });
    }

    function secondListData() {    
        return $http({
            method: 'GET',
            url: _spPageContextInfo.webAbsoluteUrl + "/_api/web/lists/getbytitle('SecondListName')/items",
            headers: { "Accept": "application/json;odata=verbose" }
        }).then(function (response) {
            return response.data.d.results;
        }).catch(function (error) {
            console.log("Error: " + JSON.stringify(error));
        });
    }

    return {
        firstListData: firstListData,
        secondListData: secondListData
    }
}]);

app.factory('UtilService', function() {
  return {
    linkLocation: function(url, launchBehavior) { 
      window.open(url, launchBehavior);
    }
  }
});

app.controller('firstController', ['$scope', '$http', '$window', 'listDataService', 'UtilService', function ($scope, $http, $window, listDataService, UtilService) {
    listDataService.firstListData().then(function (results) {

        var firstLinksList = $scope.firstLinksList = [];

        if (results.length > 0) {
            for (var i = 0; i < results.length; i++) {

                var launchBehavior;

                if (results[i].LaunchBehavior == 'New tab') {
                    launchBehavior = '_blank';
                } else {
                    launchBehavior = '_self';
                }

                firstLinksList.push({
                    title: results[i].Title,
                    linkLocationUrl: results[i].LinkLocation.Url,
                    launchBehavior: launchBehavior
                });
            }
        }
    });
    $scope.util = UtilService;
}]);

app.controller('secondController', ['$scope', '$http', '$window', 'listDataService', 'UtilService', function($scope, $http, $window, listDataService, UtilService) {
    listDataService.secondListData().then(function (results) {

        var secondLinksList = $scope.secondLinksList = [];
        if (results.length > 0) {
            for (var i = 0; i < results.length; i++) {

                var launchBehavior;

                if (results[i].LaunchBehavior == 'New tab') {
                    launchBehavior = '_blank';  
                } else {
                    launchBehavior = '_self'; 
                }

                secondLinksList.push({
                    title: results[i].Title,
                    linkLocationUrl: results[i].LinkLocation.Url,
                    launchBehavior: launchBehavior
                });           
            }
        }
    });
    $scope.util = UtilService;
}]);

From searching I think the controllers could use a service? But not sure how to restructure them.

Edit:

The HTML is similar to what you used in the PhpFiddle, using ng-repeat - but with ng-click on the list item as well:

I've also added UtilService to show how launchBehavior is used.

<h3>First list</h3>
<ul class="promoted-links">
  <li class="item default promoted-links-item" ng-repeat="link in firstLinksList" ng-click="util.linkLocation(link.linkLocationUrl, link.launchBehavior)" title="{{link.title}}">
    <h6>{{ link.title }}</h6>
  </li>
</ul>
<h3>Second list</h3>
<ul class="promoted-links">
  <li class="item default promoted-links-item" ng-repeat="link in secondLinksList" ng-click="util.linkLocation(link.linkLocationUrl, link.launchBehavior)" title="{{link.title}}">
    <h6>{{ link.title }}</h6>
  </li>
</ul>
\$\endgroup\$
2
\$\begingroup\$

One option is to make the function returned by the factory listDataService accept a parameter for the getbytitle() value:

app.factory('listDataService', ['$http', function ($http) {
    return function firstListData(title) {
        return $http({
            method: 'GET',
            url: _spPageContextInfo.webAbsoluteUrl + "/_api/web/lists/getbytitle('"+title+"')/items",
            headers: { "Accept": "application/json;odata=verbose" }
        }).then(function (response) {
            return response.data.d.results;
        }).catch(function (error) {
            console.log("Error: " + JSON.stringify(error));
        });
    };
})

Then the callbacks that process the data can be abstracted to a function. The function below replaces the for loop with the functional approach of Array.map().

$scope.processResults = function(results) {
    return results.map(function(result) {
        //consideration for launchBehavior?
        // that variable didn't appear to be used...
        return {
            title: result.Title,
            linkLocationUrl: result.LinkLocation.Url
        };
    });
};

Note: the variable launchBehavior didn't appear to be used, so if that is necessary, you will need to update that.

And those can be used to clean up the code to fetch the lists of items:

listDataService('ListName').then(function(results) {                    
    $scope.firstLinksList = $scope.processResults(results);
});
listDataService('SecondListName').then(function(results) {                    
    $scope.secondLinksList = $scope.processResults(results);
});

See a demonstration in this PHPfiddle (PHP mostly used to generate lists depending on the URL parameters). Note: I only added one single main controller... if you need to have two separate child controllers that we can look at keeping that constraint.

Arrow functions could be used to make those functions even shorter. For example, the processResults function can be simplified as below:

$scope.processResults = (results) => results.map((result) => {
    return {
        title: result.Title,
        linkLocationUrl: result.LinkLocation.Url
    };
});
\$\endgroup\$
  • \$\begingroup\$ This is much better Sam - I like the arrow functions. I updated the question but don't think we'll need to change anything? Thanks for your time and help! \$\endgroup\$ – Wayne Dec 18 '17 at 10:42

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.