angularjs-in-action / angello

MIT License
247 stars 136 forks source link

Poor usages of $scope in MainCtrl and HMTL #22

Open ThomasBurleson opened 10 years ago

ThomasBurleson commented 10 years ago

Consider the code:

myModule.controller('MainCtrl', function ($scope, AngelloModel, AngelloHelper) {
    $scope.currentStory = null;
    $scope.currentStatus = null;
    $scope.currentType = null;
    $scope.editedStory = {};

    $scope.types = AngelloModel.getTypes();
    $scope.statuses = AngelloModel.getStatuses();
    $scope.stories = AngelloModel.getStories();
    $scope.typesIndex = AngelloHelper.buildIndex($scope.types, 'name');
    $scope.statusesIndex = AngelloHelper.buildIndex($scope.statuses, 'name');

    $scope.setCurrentStory = function (story) {
        $scope.currentStory = story;
        $scope.editedStory = angular.copy($scope.currentStory);

        $scope.currentStatus = $scope.statusesIndex[story.status];
        $scope.currentType = $scope.typesIndex[story.type];
    };

    $scope.createStory = function () {
        AngelloModel.createStory($scope.editedStory);
        $scope.resetForm();
    };

    $scope.setCurrentStatus = function (status) {
        $scope.editedStory.status = status.name;
    };

    $scope.setCurrentType = function (type) {
        $scope.editedStory.type = type.name;
    };

});
ThomasBurleson commented 10 years ago

It is always a bad idea to use your $scope as a model container and manipulate the data using $scope references. Instead create separate models and publish those to the $scope.

  1. Use Controller as vm syntax to enforce concept that UI binds to a vm (view model)
  2. Create a contain object for all the current information.
  3. $scope assignments are immediate and understandable

See

myModule.controller('MainCtrl', function( models, utils ) 
{
    var statusIndices = utils.buildIndex(models.statuses, 'name'),
        typeIndices   = utils.buildIndex(models.types   , 'name'),
        vm = this;

    vm.current          = models.current;       // Data
    vm.stories          = models.stories;
    vm.types            = models.types;
    vm.statuses         = models.statuses;

    vm.select            = onSetAsCurrent;         // Event Handlers
    vm.createStory      = onCreateStory;
    vm.setCurrentStatus = onSetCurrentStatus;
    vm.setCurrentType   = onSetType;

    // *************************************
    // Private Event Handlers
    // *************************************

    function onSetAsCurrent(story) 
    {
        var current = models.current;

        current.story  = story;

        current.status = statusIndices[ story.status ];
        current.type   = typeIndices[ story.type ];
    };

    function onCreateStory() 
    {
        models.createStory($scope.editedStory);
        resetForm();
    };

    function onSetCurrentStatus( status, current ) 
    {
        current = current || models.current;
        if ( current  ) current.status = status.name;

    };

    function onSetType(type, current) 
    {
        current = current || models.current;
        if (  current ) current.type = type.name;
    };

});
ThomasBurleson commented 10 years ago

Additionally - with the use of Controller as syntax, your HTML is considerable more easy to understand:

<div ng-controller="MainCtrl as vm">
        <div class="span4 sidebar-content">
            <h2>Stories</h2>
            <div class="story" ng-repeat="story in vm.stories" ng-click="vm.select(story)">
                <h4>{{story.title}}</h4>
                <p>{{story.description}}</p>
            </div>
            <br>
            <a class="btn" ng-click="vm.createStory()" ><i class="icon-plus"></i></a>
        </div>
</div>