2
\$\begingroup\$

I did a small countdown clock challenge in JavaScript. Please review it, thanks.

const timeControls = document.querySelector('.timer__controls');
const timeLeft = document.querySelector('.display__time-left');
const endTime = document.querySelector('.display__end-time');
let interval;

function secondsToHourMinSec(seconds) {
  const date = new Date(0);
  date.setSeconds(seconds);
  return date.toISOString().substr(11, 8);
}

function advanceCurrentTime(seconds) {
  const date = new Date(0);
  date.setSeconds(Math.floor(Date.now() / 1000) + seconds);
  return date.toLocaleTimeString().substr(0, 7);
}

function displayRemainingTime(seconds) {
  timeLeft.textContent = secondsToHourMinSec(seconds);
}

function displayEndTime(seconds) {
  endTime.textContent = `Be back at ${advanceCurrentTime(seconds)}`;
}

function displayTime(seconds) {
  displayRemainingTime(seconds);
  displayEndTime(seconds);

  interval = setInterval(function() {
    seconds--;
    if (seconds < 0) {
      clearInterval(interval);
      return;
    }
    displayRemainingTime(seconds);
  }, 1000);
}

function startTimer(seconds) {
  clearInterval(interval);
  displayTime(seconds);
}

timeControls.addEventListener('click', e => {
  const button = e.target.closest('button');
  if (button)
    startTimer(parseInt(button.dataset.time));
});

document.customForm.addEventListener('submit', function(e) {
  e.preventDefault();
  const value = this.minutes.value;
  if (value.length !== 0)
    startTimer(parseInt(value) * 60);
  this.reset();
});
html {
  box-sizing: border-box;
  font-size: 10px;
  background: #8e24aa;
  background: linear-gradient(45deg, #42a5f5 0%, #478ed1 50%, #0d47a1 100%);
}

*,
*:before,
*:after {
  box-sizing: inherit;
}

body {
  margin: 0;
  text-align: center;
  font-family: "Inconsolata", monospace;
}

.display__time-left {
  font-weight: 100;
  font-size: 20rem;
  margin: 0;
  color: white;
  text-shadow: 4px 4px 0 rgba(0, 0, 0, 0.05);
}

.timer {
  display: flex;
  min-height: 100vh;
  flex-direction: column;
}

.timer__controls {
  display: flex;
}

.timer__controls>* {
  flex: 1;
}

.timer__controls form {
  flex: 1;
  display: flex;
}

.timer__controls input {
  flex: 1;
  border: 0;
  padding: 2rem;
}

.timer__button {
  background: none;
  border: 0;
  cursor: pointer;
  color: white;
  font-size: 2rem;
  text-transform: uppercase;
  background: rgba(0, 0, 0, 0.1);
  border-bottom: 3px solid rgba(0, 0, 0, 0.2);
  border-right: 1px solid rgba(0, 0, 0, 0.2);
  padding: 1rem;
  font-family: "Inconsolata", monospace;
}

.timer__button:hover,
.timer__button:focus {
  background: rgba(0, 0, 0, 0.2);
  outline: 0;
}

.display {
  flex: 1;
  display: flex;
  flex-direction: column;
  align-items: center;
  justify-content: center;
}

.display__end-time {
  font-size: 4rem;
  color: white;
}
<!DOCTYPE html>
<html lang="en">

<head>
  <meta charset="UTF-8">
  <title>Countdown Timer</title>
  <link href="https://fonts.googleapis.com/css?family=Inconsolata" rel="stylesheet" type="text/css">
  <link rel="stylesheet" href="style.css">
</head>

<body>
  <div class="timer">
    <div class="timer__controls">
      <button data-time="20" class="timer__button">20 Secs</button>
      <button data-time="300" class="timer__button">Work 5</button>
      <button data-time="900" class="timer__button">Quick 15</button>
      <button data-time="1200" class="timer__button">Snack 20</button>
      <button data-time="3600" class="timer__button">Lunch Break</button>
      <form name="customForm" id="custom">
        <input type="text" name="minutes" placeholder="Enter Minutes">
      </form>
    </div>
    <div class="display">
      <h1 class="display__time-left"></h1>
      <p class="display__end-time"></p>
    </div>
  </div>

  <script src="scripts.js"></script>
</body>

</html>

\$\endgroup\$
1
  • \$\begingroup\$ Looks great to me. \$\endgroup\$ Commented Nov 21, 2020 at 14:44

1 Answer 1

Reset to default
1
\$\begingroup\$

I have the following ideas for improvement of your code:

  • Separate different concepts like timers and UI rendering to make them easy to understand and change.
  • Use milliseconds for consistency with browser API.
  • Separate formatting and calculation for dates and durations.
  • Incapsulate mutable state in objects so it won't leak in your main application logic (let interval).
  • Don't use the substr function to format time because this solution is not robust. Write custom formatter or use a library like momentjs instead.

Here is my solution:

class Timer {
    constructor(duration, delay = 1000) {
        this._remaining = duration;
        this._delay = delay;
        this._callbacks = [];
    }

    start() {
        this._intervalId = setInterval(this._tick.bind(this), this._delay);
    }

    stop() {
        clearInterval(this._intervalId);
    }

    onTick(callback) {
        this._callbacks.push(callback);
    }

    _tick() {
        this._remaining -= this._delay;
        if (this._remaining <= 0) this.stop();
        this._callbacks.forEach((cb) => cb(this._remaining));
    }
}

class Timers {
    makeActive(timer) {
        this._activeTimer = timer;
    }

    stopActive() {
        if (this._activeTimer) this._activeTimer.stop();
    }
}

// I didn't change formatting implementation here because I am lazy
const formatHMS = (date) => date.toISOString().substr(11, 8);
const formatLocaleHMS = (date) => date.toLocaleTimeString().substr(0, 8);

const dateIn = (duration) => {
    const date = new Date(0);
    date.setMilliseconds(Date.now() + duration);
    return date;
}

const durationToDate = (duration) => {
    const date = new Date(0);
    date.setMilliseconds(duration);
    return date;
}

const timeControls = document.querySelector('.timer__controls');
const timeLeft = document.querySelector('.display__time-left');
const endTime = document.querySelector('.display__end-time');

const displayRemaining = (duration) => {
    timeLeft.textContent = formatHMS(durationToDate(duration));
}

const displayEnd = (duration) => {
    endTime.textContent = `Be back at ${formatLocaleHMS(dateIn(duration))}`;
}

const timers = new Timers();

const startAndUpdate = (duration) => {
    displayRemaining(duration);
    displayEnd(duration);

    const timer = new Timer(duration);
    timer.start();
    timer.onTick(displayRemaining);

    timers.stopActive();
    timers.makeActive(timer);
};

const secondsToMs = (seconds) => seconds * 1000;
timeControls.addEventListener('click', (e) => {
    const button = e.target.closest('button');
    if (button) startAndUpdate(secondsToMs(parseInt(button.dataset.time)));
});

const minutesToMs = (minutes) => secondsToMs(minutes * 60);
document.customForm.addEventListener('submit', function (e) {
    e.preventDefault();
    const value = this.minutes.value;
    if (value !== '') startAndUpdate(minutesToMs(parseInt(value)));
});
\$\endgroup\$
10
  • 1
    \$\begingroup\$ Using underscore to indicate private properties (though popular) is unsafe and is poor naming. Private must be enforced at a language level or it is pointless. Without enforcement you can not trust state and thus must code accordingly (you should consider them pubic and untrustable) \$\endgroup\$ Commented Nov 22, 2020 at 12:09
  • 2
    \$\begingroup\$ I use closure inside object factories with heavy reliance on getters and setters to ensure a trustable state. The reason I made a comment is that the interval timer handle once lost can not be rediscovered (well not easily, it can be guessed but at the risk of stopping other intervals) . As the code stands the interval timer represents a potential memory leak. Consider using setTimeout \$\endgroup\$ Commented Nov 22, 2020 at 14:06
  • 1
    \$\begingroup\$ Yes and that loss is unrecoverable, a memory leak. \$\endgroup\$ Commented Nov 22, 2020 at 14:25
  • 1
    \$\begingroup\$ Looks good. When defining objects one must consider the number of instances and how often these instances are created. Parsed functions are cached on all but the oldest browsers and closure across the functions is the shared factory's scope This question's requirements are for a single instance and as such there will be no additional overhead when compared to class syntax. I find that only when creating 1000's of objects per second (animations) does the class (prototype) syntax offer any detectable performance benefit. \$\endgroup\$ Commented Nov 23, 2020 at 8:52
  • 1
    \$\begingroup\$ @Blindman67 thank you for the advice. I will stick to the mutable state in closure in the future instead of using classes. \$\endgroup\$ Commented Nov 23, 2020 at 9:23

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.