2
\$\begingroup\$

The gist of the code is, you give it a date, it will start counting down how many days, hours, minutes and seconds left by emitting an event 'countDown' and once 0 is reached 'expired' event is emitted.

How can this code be improved?

class DateCountDown {
    listeners: Map<string, Array<Function>>;

    constructor(date: Date) {

        this.startCountDown(date);
        this.listeners = new Map();
        this.listeners.set("countDown", []);
        this.listeners.set("expired", []);
    }

    on(eventName: "countDown", listener: (values: CountDownValues) => void);
    on(eventName: "expired", listener: (isExpired: boolean) => void);
    on(eventName: string, listener: Function) {
        this.listeners.get(eventName).push(listener);
    }


    startCountDown(date: Date) {
        const thousand = 1000;
        const sixty = 60;
        const twentyfour = 24;

        const timer = setInterval(() => {
            const now = new Date().getTime();
            const t = date.valueOf() - now;

            const days = Math.floor(t / (thousand * sixty * sixty * twentyfour));
            const hours = Math.floor((t % (thousand * sixty * sixty * twentyfour)) / (thousand * sixty * sixty));
            const minutes = Math.floor((t % (thousand * sixty * sixty)) / (thousand * sixty));
            const seconds = Math.floor((t % (thousand * sixty)) / thousand);

            if (t <= 0) {
                clearInterval(timer);
                this.listeners.get("expired").forEach(listener =>
                    listener(true));
                return;
            }

            this.listeners.get("countDown").forEach(listener =>
                listener(new CountDownValues(days, hours, minutes, seconds))
            );
        }, 1000);
    }
}
    class CountDownValues {
    days: number;
    hours: number;
    minutes: number;
    seconds: number;

    constructor(days: number, hours: number, minutes: number, seconds: number) {
        this.days = days;
        this.hours = hours;
        this.minutes = minutes;
        this.seconds = seconds;
    }

}
var date = new Date();
date.setSeconds(date.getSeconds() + 20);

let countDown = new DateCountDown(date);

countDown.on("countDown", values => {
    console.log("from first", values);
});
countDown.on("expired", isExpired => {
    console.log("expired:", isExpired);
});
\$\endgroup\$
1
\$\begingroup\$

First, taking a look at your code as a consumer of your API:

  1. I like that the autocomplete for .on(" shows what events I can listen to.

  2. How do I stop listening to an event? If I don't want countDown events anymore then I have to do countDown.listeners.get("countDown")!.splice(index, 1) which is ugly.

  3. How do I cancel a countdown?

  4. Why is listeners exposed? This is an internal property of your class that I shouldn't need to know about as a consumer. It should be either private or protected. It's also probably worth marking startCountDown as private or protected.

  5. Why does the expired event pass an extra parameter, isExpired? If the expired event is fired, doesn't that imply that the countdown is expired?

  6. The countDown event is not fired every second. What happened to second 9 and 6?

    This is a common problem with timers, and is present in (almost) every JavaScript clock/timer question on this site. There are several possible solutions, the simplest of which is to check the time more frequently. The MDN notes on it is worth reading.

    [![broken countdown ticks][1]][1]

TypeScript specific notes:

  1. Turn on strict null checks. Yesterday. Without it, TypeScript ignores a whole class of errors.

  2. Turn on no implicit any. Once turned on this will point out that the on method has a return type of any

  3. Don't create a class to just hold properties. An interface is fine.

    interface CountDownValues {
        days: number;
        hours: number;
        minutes: number;
        seconds: number;
    }
    
  4. Consider initializing properties where they are declared when using classes.

Other:

  1. new Date().getTime() can be more concisely written as Date.now()

  2. The thousand, sixty, and twentyfour variables make it harder to read your code. Get rid of them. You can make the code more readable by instead defining day, hour, etc.

  3. Countdown is a word, you don't need to capitalize the D.

  4. Using a map for events here is probably overkill. There are only two events and they are both known at compile time. I'd just use an object.

  5. countDown is kind of an odd name for an event, without looking at your code I had no idea when it would be fired. I prefer tick, but there are other good options.

Rewrite:

I kept the feature set mostly the same, but wanted to add the ability to stop the timer, remove event listeners, and initialize a countdown before starting it. I let this run for ~15 minutes, and it never skipped a second or repeated one twice, which isn't conclusive, but I'm fairly confident it is stable.

Note that instead of firing the tick function multiple times per second, I do some math to figure out how long to wait and then schedule a tick based on that. This results in some more complicated code, but avoids the need to track what the last reported second was.

interface TickEvent {
    days: number;
    hours: number;
    minutes: number;
    seconds: number;
}

interface CountdownEvents {
    tick(values: TickEvent): void;
    expired(): void;
    stop(): void;
}

// Unfortunately we can't use T[K][] without getting messy.
type EventMap<T> = { [K in keyof T]: Function[] };

class Countdown {
    private listeners: EventMap<CountdownEvents> = { tick: [], expired: [], stop: [] };
    private timer?: number;

    on<K extends keyof CountdownEvents>(eventName: K, listener: CountdownEvents[K]): void {
        this.listeners[eventName].push(listener);
    }

    off<K extends keyof CountdownEvents>(eventName: K, listener: CountdownEvents[K]): void {
        const listeners = this.listeners[eventName];
        const index = listeners.indexOf(listener);
        if (index !== -1) {
            listeners.splice(index, 1);
        }
    }

    start(date: Date) {
        const end = Math.floor(date.getTime() / 1000);

        const tick = () => {
            const now = Date.now();
            const nowSec = Math.floor(now / 1000);
            const time = end - nowSec;

            if (time <= 0) {
                delete this.timer;
                this.listeners.expired.forEach(listener => listener());
                return;
            }

            const minute = 60;
            const hour = minute * 60;
            const day = hour * 24;

            const days = Math.floor(time / day);
            const hours = Math.floor(time % day / hour);
            const minutes = Math.floor(time % hour / minute);
            const seconds = time % minute;

            this.listeners.tick.forEach(listener => listener({ days, hours, minutes, seconds }));

            const timeToNextSecond = (nowSec + 1) * 1000 - now;
            this.timer = setTimeout(tick, timeToNextSecond);
        }

        tick();
    }

    stop() {
        if (this.timer) {
            clearTimeout(this.timer);
            delete this.timer;
            this.listeners.stop.forEach(listener => listener());
        }
    }
}

const countdown = new Countdown();
countdown.on("tick", event => console.log("tick", event));
countdown.on("expired", () => console.log("expired"));
countdown.on("stop", () => console.log("stopped"));

const date = new Date();
date.setSeconds(date.getSeconds() + 20);
countdown.start(date);

setTimeout(() => countdown.stop(), 3 * 1000);
\$\endgroup\$
  • \$\begingroup\$ Perfect, will use ur rewrite and follow all the notes going forward for ts development. Thanks again for taking the time to write such a detailed reply/review \$\endgroup\$ – Zoinky Mar 15 at 3:36

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.