3
\$\begingroup\$

I've built a countdown directive for a website I'm working on, and it actually functions just fine. It simply parses a given date, splits the total time remaining until that date up into Day, Hour, Minute, & Second components places them into the DOM, and then increments down until 0 is reached. What I'm not sure of is whether it's "fine" according to best practices and performance.

You can ignore the code related to specificity; that's just a way of determining whether a countdown should occur or not.

(function() {
    var app = angular.module('app');

    app.directive('countdown', ['$interval', function($interval) {
        return {
            restrict: 'E',
            scope: {
                specificity: '=',
                countdownTo: '=',
                callback: '&'
            },
            link: function($scope) {

                $scope.isDateExact = ($scope.specificity == 6 || $scope.specificity == 7);

                $scope.$watch('specificity', function(newValue) {
                    $scope.isDateExact = (newValue == 6 || newValue == 7);
                });

                (function() {
                    if ($scope.isDateExact) {

                        $scope.launchUnixSeconds = moment($scope.countdownTo).unix();    

                        $scope.countdownProcessor = function() {

                            var launchUnixSeconds = $scope.launchUnixSeconds;
                            var currentUnixSeconds = Math.floor($.now() / 1000);

                            if (launchUnixSeconds >= currentUnixSeconds) {
                                $scope.secondsAwayFromLaunch = launchUnixSeconds - currentUnixSeconds;

                                var secondsBetween = $scope.secondsAwayFromLaunch;
                                // Calculate the number of days, hours, minutes, seconds
                                $scope.days = Math.floor(secondsBetween / (60 * 60 * 24));
                                secondsBetween -= $scope.days * 60 * 60 * 24;

                                $scope.hours = Math.floor(secondsBetween / (60 * 60));
                                secondsBetween -= $scope.hours * 60 * 60;

                                $scope.minutes = Math.floor(secondsBetween / 60);
                                secondsBetween -= $scope.minutes * 60;

                                $scope.seconds = secondsBetween;

                                $scope.daysText = $scope.days == 1 ? 'Day' : 'Days';
                                $scope.hoursText = $scope.hours == 1 ? 'Hour' : 'Hours';
                                $scope.minutesText = $scope.minutes == 1 ? 'Minute' : 'Minutes';
                                $scope.secondsText = $scope.seconds == 1 ? 'Second' : 'Seconds';

                            }

                            if ($scope.callback && typeof $scope.callback === 'function') {
                                $scope.callback();
                            }
                        };

                        $interval($scope.countdownProcessor, 1000);
                    } else {
                        $scope.countdownText = $scope.countdownTo;
                    }
                })();

            },
            templateUrl: '/js/templates/countdown.html'
        }
    }]);
})();

The issue I'm facing is that, occasionally, I'll look at this code and think to myself "this doesn't look like other examples of Angular code I've seen" - which makes me second guess just how performant and/or how correct this code is with respect to general Angular coding style.

Overall, my JavaScript is okay. I'm not very familiar with best coding style when it comes to JS, or how to optimize for performance. Because of this, I don't have sufficient knowledge on how to analyze the performance of a piece of code.

What I do know

  • Angular has a digest cycle, and calls outside of this cycle are classed as "expensive".
  • You can optimize the performance of DOM manipulation and animations by making use of requestAnimationFrame. I am not using this.
  • Promises are heavily used in Angular. I am not using these here.

What I want to know

  • Is this an "expensive" piece of code? Does it violate Angular's digest cycle, and/or can it be improved by making use of requestAnimationFrame or promises?
  • Right now, this code's ability to callback is... poor. Not defining a callback function in the directive causes a few problems in other parts of my application.
\$\endgroup\$
3
\$\begingroup\$

First, I would suggest to move the link function to the last of the configuration. It tends to become really long, and any config placed after it might be missed which is bad for readability. That means templateUrl should be moved up.

Now, I'm not sure why you're putting most of your link code in an IIFE. It's unnecessary. The only difference an IIFE makes is it creates a new scope. Other than that, there is fundamentally no difference. You should remove it.

$scope.isDateExact = ($scope.specificity == 6 || $scope.specificity == 7);

What are 6 and 7 exactly? Why specifically 6 and 7?From another developer's perspective, they look like arbitrary numbers that don't mean anything. Nobody will ever know what 6 and 7 are! It would be better if you defined a constant or a pseudo-constant in the directive with a good name to describe what these numbers are.

Additionally, as much as possible, use strict comparison (===, !==) instead of loose comparison. You don't want to be in a situation where 6 == '6' and confuse yourself whether or not you should be providing a number or a string or both.

$scope.isDateExact = ($scope.specificity == 6 || $scope.specificity == 7);

$scope.$watch('specificity', function(newValue) {
  $scope.isDateExact = (newValue == 6 || newValue == 7);
});

if ($scope.isDateExact) {

Not sure why you'd be watching specificity and changing isDateExcact. Afaik, link is only called once during the lifetime of the directive, so essentially, the watch that changes the value of isDateExact doesn't affect anything. I can only assume that isDateExact is being updated with a watch is because the template uses isDateExact. That would mean it's template logic and it shouldn't belong in link.

<div ng-show="{{ specificity == 6 || specificity == 7 }}">

Either way, isDateExact can be simplified into an accessor function which both template and your condition can use. Both template logic and accessor should be a performance plus since you're now one less expression to watch in the digest cycle.

$scope.isDateExact = function(){
  return $scope.specificity === 6 || $scope.specificity === 7;
}

// Conditional
if($scope.isDateExact()){...

// Template
<div ng-show="{{ isDateExact() }}">

So you use moment.js. Just so you know, that library is quite heavy in terms of performance. I once profiled my apps that used moment.js and you would be surprised how much trouble it goes to give you a perfectly parsed date. And you only use it to parse the countdownTo, whatever that value is.

$scope.launchUnixSeconds = moment($scope.countdownTo).unix();

// to

$scope.launchUnixMs = Date.parse($scope.countdownTo);

I suggest that countdownTo be one of the easier formats for the native Date.parse() to parse. Most projects I encountered used the ISO-8601 format. This means, if you use that format, you can simply replace the above code with Date.parse() and totally remove the dependency to moment.js.

I also recommend you operate in milliseconds instead of seconds. Most JS date functions operate in milliseconds, so it might be easier to deal with if you just operated in milliseconds. Also, for future-proofing, it's easier to provide seconds when you're given milliseconds, rather than be given seconds initially and told to support milliseconds. That's a lot of code to change for that little requirement change.

$.now()

Now I wouldn't rely on that $ as it is not really verbose as to what it really does. $ could be jQuery, and now a plugin to shortcut Date.now(). It could also be a browser-specific shortcut. The native $ in Chrome is a shortcut to querySelector. I suggest you use Date.now() to be more verbose in your intent.

Math.floor($.now() / 1000);

// to

($.now() / 1000) | 0;

I believe you just want to truncate the decimals. Appending | 0 truncates the decimal place. Internally, it just turned the number representation from a float into an int, thus dropping decimal information. It's less overkill than Math.floor relatively but your mileage may vary.

Now I just noticed that your countdownProcessor is inside a conditional statement. Why? A function is inert unless called. You can just have countdownProcessor defined outside the condition. No reason to nest it inside an if statement.

Also, your interval keeps on running even when it hit the countdown. You should clear that interval when the countdown is over.

$scope.days = Math.floor(secondsBetween / (60 * 60 * 24));
secondsBetween -= $scope.days * 60 * 60 * 24;

I think the math here can be better represented by constants. Something like:

// Expressed in ms
var SECOND = 1000;
var MINUTE = SECONDS * 60;
var HOUR   = MINUTES * 60;
var DAY    = HOUR * 24;

Is this an "expensive" piece of code?

"expensive" is very subjective. While you can eyeball the code and say a forEach is slower than a for, it depends when it's being used. If say the code is "hot", like say in a requestAnimationFrame routine that has to finish in 16ms or less, then I'd avoid forEach. But if the code is not "hot", like say in a setup operation, then I'd use forEach to be more concise.

Does it violate Angular's digest cycle?

Another subjective question. That really depends. But a general rule of thumb I follow is if I can find a way to avoid using a watch, I'd really use it. Take advantage of the two way binding.

Can it be improved by making use of requestAnimationFrame or promises?

requestAnimationFrame is just another way to loop over code. It's the "loop" in the loop, check, update, render, repeat cycle. The cycle looks familiar, because it's similar to how angular's digest cycle works. So rAF would be like a duplication of effort.

As for promises, it won't make a difference. Promises are just a means to hold an operation's state and inform listeners when you change that state (resolve, reject). Your directive is already holding that state for you and two-way binding is doing the informing for you.

Right now, this code's ability to callback is... poor. Not defining a callback function in the directive causes a few problems in other parts of my application.

Angular has a noop function. You can always assign it as default:

// callback will always be a function
var callback = $scope.callback || angular.noop;
\$\endgroup\$
  • \$\begingroup\$ Appreciate the response! I've taken all your pointers and implemented them, except the moment date parse. The incoming date is not quite in ISO8601 (it has a space where the T should be - databases suck). I figure it's more readable to just use moment than to split and replace and then parse using the native Date syntax. \$\endgroup\$ – ReactingToAngularVues Oct 6 '15 at 0:28
  • \$\begingroup\$ Also, any tips re: requestAnimationFrame and promises? \$\endgroup\$ – ReactingToAngularVues Oct 6 '15 at 0:28
  • \$\begingroup\$ @EchoLogic Ahh, I missed the questions. Will answer in a bit. \$\endgroup\$ – Joseph Oct 6 '15 at 12:06

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.