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. Join them; it only takes a minute:

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 2 down vote favorite

I wrote a script for a timer. Is there any way to improve the code, with fewer number of lines?

JSBIN

var app = angular.module('app', []);
app.controller('main', function($scope, $interval) {

    $scope.timer = 0;
    var clickme = true,
        startfun;
    $scope.status = "Start";
    var foo = function() {
        $scope.timer += 1;
    };
    $scope.start = function() {
        if (clickme === true) {
            $scope.status = "Stop";
            clickme = false;
            startfun = $interval(foo, 1000);

        } else if (clickme === false) {
            clickme = true;
            $interval.cancel(startfun);
        }

    };
  $scope.reset=function(){

    $interval.cancel(startfun);
    $scope.timer = 0;
    $scope.status = "Start";
    clickme = true;
  };
});
share|improve this question
up vote 5 down vote accepted

Aside from the issue of setIntervals resolution/accuracy there's a lot of room for improvement in your angular.

angular.module('app', []);

"app" isn't a good name for your module "fruitjs.timer" would be better as it is more descriptive and has a much lower chance of naming clashes.

app.controller('main', function($scope, $interval) {

Again "main" isn't a great name for your controller "timerController" would be more explicit.

You should also utilise angular's support for safe minification:

app.controller('main', ['$scope', '$interval', function($scope, $interval) {
    // code
}]);

Notice that the second parameter is now an array declaring the names of the services/modules you require and finally your controller function (this is the recommended way).

More bad naming clickme, foo, startfun - none of these names mean anything.

$scope.status isn't the status at all - it's the potential action.

You don't need the clickme variable (which should at least be camel cased to clickMe. You have 2 potential actions "Start" and "Stop" - you don't need the additional boolean to track what state you're currently in.

Use either 2 or 4 spaces for your indentation - don't mix and match.

You don't change the label after you stop the timer.

Code with above changes

var app = angular.module('fruitjs.timer', []);
app.controller('timerController', [ '$scope', '$interval', function($scope, $interval) {
    var interval, incrementTimer, actions;
    actions = { start: "Start", stop: "Stop" };

    $scope.timer = 0;
    $scope.action = actions.start;

    incrementTimer = function() {
        $scope.timer += 1;
    };

    $scope.toggle = function() {
        if ($scope.action === actions.start) {
            $scope.action = actions.stop;
            interval = $interval(incrementTimer, 1000);

        } else if ($scope.action === actions.stop) {
            $scope.action = actions.start;
            $interval.cancel(interval);
        }
    };

    $scope.reset = function () {
        $interval.cancel(interval);
        $scope.timer = 0;
        $scope.action = actions.start;
    };
}]);

All you need to do now is solve the problem of keeping time accurately :)

share|improve this answer
1  
I like this review. As this is "The Angular Way", I would encapsulate the State/Timer functionality in a Service and also have the incrementTimer() function calculate the number of seconds elapsed since last switching to "Running". Finally, the $scope.status and $scope.timer properties should be made into functions that point to this service and let the digest loop do its thing. – malix Apr 29 '15 at 18:45
    
@malix several good points there - you could turn it into an answer :) – RobH Apr 30 '15 at 8:04
up vote 3 down vote

$interval is a wrapper for window.setInterval(), and window.setInterval() does not provide an accurate tick. As stated in the Mozilla documentation for that and the related window.setTimeout(),

  • The timeout can fire later when the page (or the OS/browser itself) is busy with other tasks.
  • Timeouts are clamped to firing no more than once a second in inactive tabs. Indeed, on Safari in OS X, I see that your timer runs at about half speed if I let it run in a background tab. See also this complaint.
  • If the entire computer (or virtual machine) is suspended, it will obviously miss ticks.

Therefore, you cannot just increment a counter for your clock. The right thing to do is to record the start time, and on each callback, report the difference between the current time and the start time as the elapsed time.

share|improve this answer
    
Thanks for the inputs and I will try to create a better timer in future which will work on elapsed time btw start and current time. – fruitjs Apr 30 '15 at 3:32
up vote 1 down vote

As suggested by @RobH himself, I've turned my comment into an additionnal answer:

  • As this is "The Angular Way", I would encapsulate the State/Timer functionality in a Service
  • I would have the incrementTimer() function calculate the number of seconds elapsed since last switching to "Running"
  • Finally, the $scope.status and $scope.timer properties should be made into functions that point to this Service and let the $digest() loop do its thing
share|improve this answer

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.