LiveTime Object Literal

Question

I have a website that retrieves time data from any given location on the world live.

I wanted to be able to automatically change the time and not deal with the hassle of local client times.

It's fairly simple, but since I'm not that good with JS, I'd like to know if there is anything I could improve about it?

Suggestions are welcome!

(function() {
    "use strict";

    var LiveTime = {
        rawOffset: null,
        dstOffset: null,
        timeoutId: null,

        setOffset: function(raw, dst) {
            LiveTime.rawOffset = raw * 1000;

            if (typeof dst !== "undefined") {
                LiveTime.dstOffset = dst * 1000;
            }
        },

        start: function() {
            LiveTime.timeoutId = (function increment() {
                var now = new Date();
                var now = new Date(
                    now.getUTCFullYear(),
                    now.getUTCMonth(),
                    now.getUTCDate(),
                    now.getUTCHours(),
                    now.getUTCMinutes(),
                    now.getUTCSeconds(),
                    now.getUTCMilliseconds()
                );

                now.setTime(now.valueOf() + LiveTime.rawOffset + LiveTime.dstOffset);

                var element = document.getElementById("current-time");
                element.innerHTML = ("0" + now.getHours()).slice(-2) + ":"
                                  + ("0" + now.getMinutes()).slice(-2) + ":"
                                  + ("0" + now.getSeconds()).slice(-2);

                LiveTime.timeoutId = setTimeout(increment, 50);
            })();
        },

        stop: function() {
            clearTimeout(LiveTime.timeoutId);
        }
    }

    //usage example
    LiveTime.start();

    //simulate new location after 5 seconds
    setTimeout(function() {
        LiveTime.setOffset(-9 * 3600, 3600); //alaska
   }, 5000);

    //stop the time after 5 more seconds
    setTimeout(function() {
        LiveTime.stop();
   }, 10000);
})();

<div id="current-time"></div>

konijn · Accepted Answer · 2014-10-29 13:21:36Z

From a once over:

dst stands for daylight saving time, you should comment that
offset I assume is the offset to the UTC (Greenwich time), you should comment that as well
I am ambiguous about declaring rawOffset, dstOffset, timeoutId. I can see the value of self documenting code, but is it worth it ? Not sure. The code works fine if you delete those 3 statements.

Consider falsy evaluations

if (typeof dst !== "undefined") {
  LiveTime.dstOffset = dst * 1000;
}

compared to

if (dst) {
  LiveTime.dstOffset = dst * 1000;
}

or even (a bit hacky)

LiveTime.dstOffset = dst ? dst * 1000 : 0;

Do not declare variables twice like here:

var now = new Date();
var now = new Date(

You can get the timestring this way:
```
now.toTimeString().substring(0,8);
```
This basically keeps only the first 8 (hh:mm:ss) characters of the time string
You can get the UTC date value this way:
```
new Date((new Date()).toUTCString().slice(0,-4) )        
```
This gets the UTC date, strips off the timezone part and re-creates the date
document.getElementById("current-time"); <- This should be a parameter
As @Pinoniq mentioned, ideally you should write your class/code so that you can run multiple instances.
LiveTime is not a constructor (yet), so it should be called liveTime

This would be my counter proposal:

function liveTime(elementId, offsetSeconds, dayLightTimeSavingSeconds) {
    "use strict";

    var timeZoneOffset = 0,
        daylightTimeSavingOffset = 0,
        element = document.getElementById(elementId),
        timeoutId;
  
    if( offsetSeconds ){
      setTimeOffset( offsetSeconds, dayLightTimeSavingSeconds );
    }

    function setTimeOffset(offsetSeconds, dayLightTimeSavingSeconds) {
        //Convert to milliseconds
        timeZoneOffset = offsetSeconds * 1000;
        daylightTimeSavingOffset = (dayLightTimeSavingSeconds || 0) * 1000;
    }

    function start() {
        if( timeoutId ){
          return; //We already started
        }
        (function increment() {
            //Set the time to UTC time
            var now = new Date((new Date()).toUTCString().slice(0, -4));
            //Add the configured offsets
            now.setTime(now.valueOf() + timeZoneOffset + daylightTimeSavingOffset);
            //Update the HTML
            element.innerHTML = now.toTimeString().substring(0, 8);
            //Come back again
            timeoutId = setTimeout(increment, 50);
        })();
    }

    function stop() {
        clearTimeout(timeoutId);
        timeoutId = 0;
    }
    return {
        setTimeOffset: setTimeOffset,
        start: start,
        stop: stop
    };
}

//usage example
liveTime('current-time',-9 * 3600, 3600).start();
liveTime('current-time2', 4 * 3600 ).start();

Alaska time:<span id="current-time"></span><br>
Moscow time:<span id="current-time2"></span>

Thanks for the review! Will digest this new information some time later. But since you've made it a constructed object, shouldn't the name be LiveTime instead of liveTime? Or is it not an object with a constructor? — Kid Diamond
– Kid Diamond, Commented Oct 29, 2014 at 14:34
It is not a true constructor, though it could have been called LiveTime — konijn
– konijn, Commented Oct 29, 2014 at 15:27
Anything in particular why you didn't decide to go with the "normal" constructor way? — Kid Diamond
– Kid Diamond, Commented Oct 29, 2014 at 15:29
Yes, it removes the need for this all over the code and has baked in support for splitting private/public parts. — konijn
– konijn, Commented Oct 29, 2014 at 15:39

Community · Accepted Answer · 2020-06-10 13:24:26Z

You state that it's fairly simple. It isn't...

Tight coupling

Everything in your code is tightly coupled. Even the id of the element where the time is displayed is hard coded into the code.

What if I want multiple timers next to each other? Useless.

Strange naming "convention"

You use the word offset alot. But offset to what? What is your default time? After digging around I think it is UTC?

Then there is your method that let's you change the way the time is presented: setOffset. It accepts 2 arguments. raw and dst. What the hell is raw and dst? Again I have to dig into the code and still have no idea what you are doing. I think that raw and dst have to be passed in as seconds? But what is the difference between those variables? I have no idea.

dst seems to be optional. why? No idea. In the end you always count dst and raw together everytime you use them.

Some other strange doings

you create an IIFE (immediately invoked function expression) but you give it a name (function increnment(){})();. What this piece of code does is create function called increment. It then immediately invokes that function. The return value of that invoked function is stored in LiveTime.timeoutId. Or no, wait. The invoked function itself writes to LiveTime.timeoutId. Wow, do I need to explain more?

Not to talk about the horror of increment() actually caling itself. Inception!

KISS: Keep It Stupid Simple

Every function of your code should do 1 thing and 1 thing only. And preferably it should solve a little problem.

Defining the problem

The first step in solving a problem - code should always solve a problem. If it doesn't, you don't need it - is defining the problem.

Your problem:

I wanted to be able to automatically change the time and not deal with the hassle of local client times.

Actually consists of 2 problems (note the and in your sentence). The first can be solved by using timeouts, or even better an interval. The second should have it's own method that then would be called by the interval function. This method would take care of returning the current data as UTC (for instance).

You cram everything in an thing that looks like an IIFE whos return value is stored in a variable (but evnetually the function returns nothing itself, but sets the value in the IIFE itself).

Thanks for your review. I'm using setTimeout for the function to call itself, because if I use setInterval the execution of the method would get queued up after some time. Also, isn't the point of object literals for it to be only 1 instance (singleton)? This LiveTime object literal is not meant to have multiple instances, that's why I've tightly coupled it with my HTML code. — Kid Diamond
– Kid Diamond, Commented Oct 28, 2014 at 14:41
That still doesn"t mean it's good. In a few month you will look back an regret the tight coupling — Pinoniq
– Pinoniq, Commented Oct 28, 2014 at 16:19
I'm not really worried about that since the application is really small. But basically what you're saying, is that it would be better to make this an constructed object instead of a literal? I don't see a way how I can "inject" the HTML dependency objects when using object literals. — Kid Diamond
– Kid Diamond, Commented Oct 28, 2014 at 16:23

Stack Exchange Network

LiveTime Object Literal

2 Answers 2

Tight coupling

Strange naming "convention"

Some other strange doings

KISS: Keep It Stupid Simple

Defining the problem

You must log in to answer this question.

Hot Network Questions

LiveTime Object Literal

2 Answers 2

Tight coupling

Strange naming "convention"

Some other strange doings

KISS: Keep It Stupid Simple

Defining the problem

You must log in to answer this question.

Related

Hot Network Questions