1
\$\begingroup\$

I've asked question on SO how to rewrite jQuery code to Angular. Because I got no answers I've tried to create something by my own. This is my version of code:

(function() {
  'use strict';

  angular
    .module('misiu.directives', [])
    .directive('console', ['$rootScope', function($rootScope) {
      return {
        restrict: 'E',
        template: [
          '<div class="console animated">',
          '<div ng-repeat="event in events" ng-class="{\'error\': event.type == \'error\', \'log\': event.type == \'log\'}">',
          '{{event.data}}<span ng-if="event.count>1" class="count">{{event.count}}</span>',
          '</div>',
          '</div>'
        ].join(''),
        link: function($scope, element) {

          $scope.events = [];

          $rootScope.$on('Log', function(event, data) {

            if ($scope.events[0] !== undefined && $scope.events[0].type === "log" && $scope.events[0].data === "Log :: " + data) {
              $scope.events[0].count++;
            } else {
              $scope.events.splice(0, 0, {
                type: 'log',
                data: "Log :: " + data,
                count: 1
              });
            }

          });

          $rootScope.$on('Error', function(event, data) {

            if ($scope.events[0] !== undefined && $scope.events[0].type === "error" && $scope.events[0].data === "Error :: " + data) {
              $scope.events[0].count++;
            } else {
              $scope.events.splice(0, 0, {
                type: 'error',
                data: "Error :: " + data,
                count: 1
              });
            }

          });
        }
      };
    }])
})();

and Plunker showing demo (move mouse over console div to add entries)

Because this is my first directive I'd like to ask for code review.

I'm adding new items to my console using one of those lines:

$rootScope.$emit("Log", "Something");
$rootScope.$emit("Error", "Terrible error");

I must add $rootScope as dependency to every controller if I want to use my directive there. Maybe factory would be better? This way I could for example add a way to remove everything except last 20 entries.

I'd like my directive to be optimized for speed. I've tested it for a while, but I'm not sure I did everything properly.

I need only single way binding and the thing that can change it only top most element (counter can increase), rest of element don't need binding. Maybe this can be also optimized.

Any other comments on what is wrong are welcome.

EDIT
I've created another code review request that includes same control, but using provider.

\$\endgroup\$
3
  • 1
    \$\begingroup\$ I have rolled back the last edit. Please see what you may and may not do after receiving answers. \$\endgroup\$ – Pimgd Mar 22 '16 at 16:30
  • \$\begingroup\$ @Pimgd sorry for that, I'll post new question with link to this one. \$\endgroup\$ – Misiu Mar 22 '16 at 16:32
  • 1
    \$\begingroup\$ @Pimgd I've included link to new question as You suggested. \$\endgroup\$ – Misiu Mar 22 '16 at 16:43
1
\$\begingroup\$
    template: [
      '<div class="console animated">',
      '<div ng-repeat="event in events" ng-class="{\'error\': event.type == \'error\', \'log\': event.type == \'log\'}">',
      '{{event.data}}<span ng-if="event.count>1" class="count">{{event.count}}</span>',
      '</div>',
      '</div>'
    ].join(''),

If you have a build step with a transpiler, or don't care about old browsers, consider using template strings instead. Template strings can multi-line, making it easy to build markup.

$rootScope.$on('Log', function(event, data) {

$rootScope is great for data relay. However, when an app grows, you will be caught in a tangle of events. You don't know who fired them, what they'll execute, their payload (unless you document them, which rarely happens). Plus, $rootScope is a global and we all know what happens to globals when they get polluted. Suggesting you use a service/factory instead. That way, it isn't global plus you have some form of control.

Also, instead of separate events, why not a single event. It seems like your logic already sorts itself out by the type property.

$scope.events[0] !== undefined

Looks like you're checking if the array contains something. It would be best expressed if you checked the array length property instead.

$scope.events.splice(0, 0, {
  type: 'log',
  data: "Log :: " + data,
  count: 1
});

There's array.unshift. It's like array.push but puts the new value at the start of the array.

\$\endgroup\$
4
  • \$\begingroup\$ Thank You for review :) I'll fix parts I can right away. But I don't know how to change my directive to use factory. I'd be grateful for advice on that. \$\endgroup\$ – Misiu Mar 22 '16 at 13:20
  • \$\begingroup\$ @Misiu Here's an article that briefly describes what they are as well as your options tylermcginnis.com/angularjs-factory-vs-service-vs-provider \$\endgroup\$ – Joseph Mar 22 '16 at 13:37
  • \$\begingroup\$ Thank You for link, I found similar thing on SO: stackoverflow.com/questions/15666048/… Ideally I'd like to be able to set some config, so I'll go with provider. I'll add my code to review when I'm done :) \$\endgroup\$ – Misiu Mar 22 '16 at 13:45
  • \$\begingroup\$ I've added new question with code using provider, could You please take a look at it. \$\endgroup\$ – Misiu Mar 22 '16 at 16:44

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.