current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
up vote 0 down vote favorite

I decided to write a simple runnable example to try and express my concern:

index.html

<section ng-app="App" ng-clock>
  <div ng-controller="ShopController as shop">                
      <prize-directive ng-repeat="prize in prizes"></prize-directive>
  </div>
</section>

Inside my shopController I have set a static prize list just for the purpose of keeping it simple.

ShopController.js

angular.module('app.controllers').controller('ShopController',
   ['PrizeService', '$scope', function (PrizeService, $scope) {

    $scope.prizes = [
      {name: 'First Prize', cost: 10, 
         image: 'upload/prize/image1.png', note: 'This is a note'},
      {name: 'Second Prize', cost: 20,
         image: 'upload/prize/image2.png', note: 'This is a note'},
      {name: 'Third Prize', cost: 30, 
         image: 'upload/prize/image3.png', note: 'This is a note'}
    ];

    $scope.reclaim = function (prize) {
      console.log(prize);
    };
}]);

In the original code I use the PrizeService to load a list of available prizes, but back to my main problem:

PrizeDirective

angular.module('app.directives').directive('prizeDirective', function () {
  return {    
    restrict: 'E',
    templateUrl: 'js/directives/PrizeDirective.html'
  };
});

HTML:

<div>    
    <img ng-src="{{Endpoint + prize.image}}" />
    <h3>{{prize.name}}</h3>
    <span>Pontos: {{prize.cost}}</span>    
    <p>{{prize.note}}</p>
</div>
<reclaim-directive></reclaim-directive>

Here in the HTML portion of the directive, I'm using a variable name that was defined in the index.html. Although it bothers me, I'm willing to let it go because the variable name was defined inside the ng-repeat code, which is inside the prize-directive directive. But when we go to the reclaim-directive, things get a little bit more heated:

angular.module('app.directives').directive('reclaimDirective', function () {
  return {    
    restrict: 'E',
    template: '<button ng-click="reclaim(prize)">Resgatar</button>'
  };
});

Note: I'm actually using the templateUrl here, but again, I'm simplifying it just to show my point.

Now I'm using a function name defined inside ShopController in the Reclaim Directive. Differently from OOP, this is not really an inheritance of an abstract method which even the IDE can tell you that something is wrong and, to me, it seems like a bad practice.

I would love to hear people's thoughts on this approach and if there is anything that I'm missing that could help clean up this code.

share|improve this question
up vote 2 down vote accepted

Now I'm using a function name defined inside ShopController in the Reclaim Directive... it seems like a bad practice.

Yep, this kind of code will be hard to debug and maintain.

  • You're depending on the existence of reclaim to be somewhere in the scope chain, on some controller enclosing your directive.
  • There's no indication whenever you use the directive that reclaim is missing in the scope. The directive will fail only if you click it.
  • Without proper tools like Batarang, in a highly nested controller structure, reclaim will be hard to find when you don't know which controller it resides.

If <reclaim-directive> isn't that huge, and is not used anywhere else independently, then it's contents should belong to <prize-directive>.

If <reclaim-directive> is big and independent enough to be a separate directive, then consider making it a sibling of <reclaim-directive>. That way, you avoid the extra nested level:

<div ng-controller="ShopController as shop">                
  <prize-directive ng-repeat="prize in prizes"></prize-directive>
  <reclaim-directive></reclaim-directive>
</div>

Most importantly, instead of depending on implicit existence of reclaim in the scope, why not provide it to the directive explicitly? Construct a scope for the directive, which accepts a binding of reclaim. I believe this is the way to do it:

<prize-directive ng-repeat="prize in prizes" on-reclaim="reclaim"></prize-directive>

angular.module('app.directives').directive('prizeDirective', function () {
  return {
    restrict: 'E',
    templateUrl: 'js/directives/PrizeDirective.html',
    scope: {
      onReclaim: '='
    }
  };
});

<!-- onReclaim assigned to on-reclaim comes from the prizeDirective -->
<reclaim-directive on-reclaim="onReclaim"></reclaim-directive>

angular.module('app.directives').directive('reclaimDirective', function () {
  return {    
    restrict: 'E',
    template: '<button ng-click="onReclaim(prize)">Resgatar</button>',
    scope: {
      onReclaim: '='
    }
  };
});
share|improve this answer
    
I ran into a problem in making reclaim-directive a sibling of prize-directive because the ng-repeat clause is at the prize-directive, which makes the reclaim-directive show up only once at the end (not repeat through). – Marco Aurélio Deleu Nov 7 '15 at 15:03

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.