patternjavascriptMinor
Countdown directive built using Angular
Viewed 0 times
builtcountdowndirectiveangularusing
Problem
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
```
(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.da
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.da
Solution
First, I would suggest to move the
Now, I'm not sure why you're putting most of your
What are
Additionally, as much as possible, use strict comparison (
Not sure why you'd be watching
Either way,
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
I suggest that
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 I wouldn't rely on that
I believe you just want to truncate the decimals. Appending
Now I just noticed that your
Also, your interval keeps on running even when it hit the countdown. You should clear that interval when the countdown is over.
I think the math here can be better represented by constants. Something like:
Is this an "expensive" piece of code?
"expensive" is very subjective. While you can eyeball the code and say a
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.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
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 fCode Snippets
$scope.isDateExact = ($scope.specificity == 6 || $scope.specificity == 7);$scope.isDateExact = ($scope.specificity == 6 || $scope.specificity == 7);
$scope.$watch('specificity', function(newValue) {
$scope.isDateExact = (newValue == 6 || newValue == 7);
});
if ($scope.isDateExact) {<div ng-show="{{ specificity == 6 || specificity == 7 }}">$scope.isDateExact = function(){
return $scope.specificity === 6 || $scope.specificity === 7;
}
// Conditional
if($scope.isDateExact()){...
// Template
<div ng-show="{{ isDateExact() }}">$scope.launchUnixSeconds = moment($scope.countdownTo).unix();
// to
$scope.launchUnixMs = Date.parse($scope.countdownTo);Context
StackExchange Code Review Q#106570, answer score: 3
Revisions (0)
No revisions yet.