5
\$\begingroup\$

I'm using the following pattern in my mobile app. The code snippet below manages a stopwatch.

Is there a way to simplify the code for better readability and probably getting away from needing to use MyApp.stopwatch. inside it? Somehow in some cases using this instead works, and in some it doesn't.

And what about getting rid of using repeating var settings = MyApp.stopwatch.settings?

(function (MyApp, $, undefined) {

    // Using strict mode to throw exceptions for 'unsafe' actions and coding patterns
    'use strict';

    // Initializes app
    function init() {
        MyApp.stopwatch.init('startStopwatch', 'pauseStopwatch', 'resetStopwatch');
    }

    // Manages the stopwatch
    MyApp.stopwatch = {

        init: function (startButton, pauseButton, resetButton) {
            document.getElementById(startButton).addEventListener('click', MyApp.stopwatch.startTimer, false);
            document.getElementById(pauseButton).addEventListener('click', MyApp.stopwatch.pauseTimer, false);
            document.getElementById(resetButton).addEventListener('click', MyApp.stopwatch.resetTimer, false);

            MyApp.stopwatch.displayTimer();
        },

        settings: {
            timerId: -1,
            interval: 100,
            millis: 0,
            seconds: 0,
            minutes: 0
        },

        displayTimer: function () {
            // ARE THESE REPEATING DECLARATIONS REALLY NEEDED?
            var settings            = MyApp.stopwatch.settings,
                millis              = Math.round(settings.millis / 100).toFixed(0),
                seconds             = settings.seconds,
                minutes             = settings.minutes;

            if (seconds < 10) {
                seconds             = '0' + seconds;
            }
            if (minutes < 10) {
                minutes             = '0' + minutes;
            }
            document.getElementById('stopwatch').innerHTML = minutes + ':' + seconds + ':' + millis;
        },

        updateTimer: function () {
            // ARE THESE REPEATING DECLARATIONS REALLY NEEDED?
            var settings            = MyApp.stopwatch.settings;
            settings.millis         += settings.interval;
            if (settings.millis >= 1000) {
                settings.millis     = 0;
                settings.seconds    += 1;
            }
            if (settings.seconds >= 60) {
                settings.millis     = 0;
                settings.seconds    = 0;
                settings.minutes    += 1;
            }
            MyApp.stopwatch.displayTimer();
        },

        pauseTimer: function () {
            // ARE THESE REPEATING DECLARATIONS REALLY NEEDED?
            var settings    = MyApp.stopwatch.settings;
            window.clearInterval(settings.timerId);
            settings.timerId = -1;
        },

        startTimer: function () {
            // ARE THESE REPEATING DECLARATIONS REALLY NEEDED?
            var settings    = MyApp.stopwatch.settings;
            if (settings.timerId === -1) {
                settings.timerId = window.setInterval(MyApp.stopwatch.updateTimer, settings.interval);
            }
        },

        resetTimer: function () {
            // ARE THESE REPEATING DECLARATIONS REALLY NEEDED?
            var settings    = MyApp.stopwatch.settings;
            MyApp.stopwatch.pauseTimer();
            settings.millis = 0;
            settings.seconds = 0;
            settings.minutes = 0;
            MyApp.stopwatch.displayTimer();
        }

    };

    // PhoneGap, jQuery & device is ready now -> initialize
    $(document).on('deviceready', init);

}(window.MyApp = window.MyApp || {}, jQuery));
\$\endgroup\$
2
\$\begingroup\$

This is what I came up with (untested):

I got rid of your settings-stuff, introduced a variable self so there's no interference with this, moved your initialization code to the only place it is (and can ever be) called from, extracted a method for padding and event handler registration and fixed your updateTimer so it works for values > 1000 (it's not fit for negative values, though). I hope you like it...

(function (MyApp, $, undefined) {
    // Using strict mode to throw exceptions for 'unsafe' actions and coding patterns
    'use strict';

    function addClickHandlerToButton(buttonId, handler) {
        document.getElementById(buttonId).addEventListener('click', handler, false);
    }

    function padLeft(number) {
        number = +number;
        return (number < 10 ? '0' : '') + Math.floor(number);
    }

    // Manages the stopwatch
    var stopwatch = (function(){
        var minutes = 0,
            seconds = 0,
            millis = 0,
            interval = 100,
            timerId = -1,
        displayTimer = function () {
            document.getElementById('stopwatch').innerHTML =
                padLeft(minutes) + ':' +
                padLeft(seconds) + ':' +
                Math.floor(millis);
        },

        updateTimer = function () {
            millis += interval;
            if (millis >= 1000) {
                seconds += millis / 1000;
                millis %= 1000;
            }
            if (seconds >= 60) {
                minutes += seconds / 60;
                seconds %= 60;
            }
            displayTimer();
        },

        pauseTimer = function () {
            window.clearInterval(timerId);
            timerId = -1;
        },

        startTimer = function () {
            if (timerId === -1) {
                timerId = window.setInterval(updateTimer, interval);
            }
        },

        resetTimer = function () {
            pauseTimer();
            millis = 0;
            seconds = 0;
            minutes = 0;
            displayTimer();
        };

        return function (startButton, pauseButton, resetButton) {
            addClickHandlerToButton(startButton, startTimer)
            addClickHandlerToButton(pauseButton, pauseTimer)
            addClickHandlerToButton(resetButton, stopTimer)
            displayTimer();
        };
    })();

    // PhoneGap, jQuery & device is ready now -> initialize
    $(document).on('deviceready', function() {
        stopwatch('startStopwatch', 'pauseStopwatch', 'resetStopwatch');
    });

}(window.MyApp = window.MyApp || {}, jQuery));

EDIT 1+2: updated the code to integrate suggestions from the comments by the OP and fix errors.

\$\endgroup\$
  • \$\begingroup\$ Looks good. Especially like the handler function. Never seen self.self = self; being used? Is it a good practice? And doesn't init and displayTimer have a different scope? So when you assign MyApp.stopwatch.self = MyApp.stopwatch can you really just call self.millis? Because if you can access MyApp.stopwatch with self inside any of those functions, why can't you then just call displayTimer() instead of self.displayTimer, or millis instead of self.millis \$\endgroup\$ – micadelli Aug 24 '12 at 20:40
  • \$\begingroup\$ You are right, my changes weren't fully thought through and included redundancies (and a copy paste error, I used the same button for all 3 event handler registrations). Please have another look. I also moved self - it's a neat trick, although in this version, it isn't really needed anymore. \$\endgroup\$ – Arne Aug 24 '12 at 22:14
  • \$\begingroup\$ My questions on last comment were kind of rhetorical. The code won't still work because those functions have different scope and therefore you just can't call stopTimer or millis inside them. But yeah, I'll implement that handler-function. \$\endgroup\$ – micadelli Aug 24 '12 at 22:41
  • \$\begingroup\$ Hmm... I really need a testbed :-) - just give them scope by making MyApp.stopwatch a function. I'll do another rewrite! \$\endgroup\$ – Arne Aug 25 '12 at 5:47
  • \$\begingroup\$ Although you've changed the pattern, i'll accept this as an answer. Good effort. Ty! \$\endgroup\$ – micadelli Aug 25 '12 at 16:04

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.