4
\$\begingroup\$

I have been reading Clean Code and yesterday I completed day 2 of JavaScript 30. I tried to make the JavaScript I wrote for this as clean as possible. This is probably some of the cleanest code I've ever written, but now I'm wondering how it could be even better. All suggestions are welcome. Thanks!

HTML:

<!DOCTYPE html>
<html>
<head>
  <meta charset="UTF-8">
  <title>JS + CSS Clock</title>
</head>
<body>
  <div class="clock">
    <div class="clock-face">
      <div class="hand hour-hand"></div>
      <div class="hand minute-hand"></div>
      <div class="hand second-hand"></div>
    </div>
  </div>
</body>
</html>

JavaScript:

const MILLISECONDS_PER_SECOND = 1000;
const SECONDS_PER_MINUTE = 60;
const MINUTES_PER_HOUR = 60;

const DEGREES_PER_ROTATION = 360;
const SECONDS_PER_CLOCK_ROTATION = 60;
const MINUTES_PER_CLOCK_ROTATION = 60;
const HOURS_PER_CLOCK_ROTATION = 12;

class Hand {
  constructor(element) {
    this.element = element;
    this.completedRotations = 0;
  }

  updateRotation(lastNow, now) {
    if (this.hasCompletedARotation(lastNow, now)) {
      this.completedRotations++;
    }
    const degrees = this.calculateRotationInDegrees(now);
    this.element.style.transform = `rotate(${degrees}deg)`;
  }
}

class SecondHand extends Hand {
  hasCompletedARotation(lastNow, now) {
    return lastNow.getSeconds() > now.getSeconds();
  }

  calculateRotationInDegrees(now) {
    const seconds = now.getSeconds();
    const secondsRotationProgress = seconds / SECONDS_PER_CLOCK_ROTATION;
    return (
      (secondsRotationProgress + this.completedRotations) *
      DEGREES_PER_ROTATION
    );
  }
}

class MinuteHand extends Hand {
  hasCompletedARotation(lastNow, now) {
    return lastNow.getMinutes() > now.getMinutes();
  }

  calculateRotationInDegrees(now) {
    const milliseconds = now.getMilliseconds();
    const seconds = now.getSeconds();
    const minutes = now.getMinutes();
    const millisecondsRotationProgress =
          milliseconds /
          MILLISECONDS_PER_SECOND /
          SECONDS_PER_MINUTE /
          MINUTES_PER_CLOCK_ROTATION;
    const secondsRotationProgress =
          seconds / SECONDS_PER_MINUTE / MINUTES_PER_CLOCK_ROTATION;
    const minutesRotationProgress = minutes / MINUTES_PER_CLOCK_ROTATION;
    const totalRotationProgress =
          millisecondsRotationProgress +
          secondsRotationProgress +
          minutesRotationProgress;
    return (
      (totalRotationProgress + this.completedRotations) *
      DEGREES_PER_ROTATION
    );
  }
}

class HourHand extends Hand {
  hasCompletedARotation(lastNow, now) {
    return (
      lastNow.getHours() % HOURS_PER_CLOCK_ROTATION >
      now.getHours() % HOURS_PER_CLOCK_ROTATION
    );
  }

  calculateRotationInDegrees(now) {
    const seconds = now.getSeconds();
    const minutes = now.getMinutes();
    const hours = now.getHours();
    const hoursOnClock = hours % HOURS_PER_CLOCK_ROTATION;
    const secondsRotationProgress =
          seconds /
          SECONDS_PER_MINUTE /
          MINUTES_PER_HOUR /
          HOURS_PER_CLOCK_ROTATION;
    const minutesRotationProgress =
          minutes / MINUTES_PER_HOUR / HOURS_PER_CLOCK_ROTATION;
    const hoursRotationProgress = hoursOnClock / HOURS_PER_CLOCK_ROTATION;
    const totalRotationProgress =
          secondsRotationProgress + minutesRotationProgress + hoursRotationProgress;
    return (
      (totalRotationProgress + this.completedRotations) *
      DEGREES_PER_ROTATION
    );
  }
}

const hands = [];

const start = new Date();
let lastNow;
let now = start;

setupHands();
update();

function setupHands() {
  const secondHandElement = document.querySelector('.second-hand');
  const minuteHandElement = document.querySelector('.minute-hand');
  const hourHandElement = document.querySelector('.hour-hand');

  const secondHand = new SecondHand(secondHandElement);
  const minuteHand = new MinuteHand(minuteHandElement);
  const hourHand = new HourHand(hourHandElement);
  hands.push(secondHand, minuteHand, hourHand);
}

function update() {
  updateTimestamps();
  updateClockHands();
  window.requestAnimationFrame(update);
}

function updateTimestamps() {
  lastNow = now;
  now = new Date();
}

function updateClockHands() {
  hands.forEach(hand => hand.updateRotation(lastNow, now));
}

JSBin with everything

\$\endgroup\$
1
  • \$\begingroup\$ I found a few improvements myself which have been made here. Namely I got rid of the unused start variable, replaced the lastNow variable with a lastUpdated field on the Hand class, and renamed the update function to animationLoop. \$\endgroup\$
    – tlent
    Commented Nov 12, 2017 at 19:29

1 Answer 1

2
\$\begingroup\$

Too complex.

There is a very simple rule in all programming, KISS (Keep It Simple Simon (*)) and your code has strayed a long way from that rule.

Complexity is a breeding ground of problems, it is hard to maintain, more likely to be buggy, can be resource hungry, and more.

You started well by defining the hand, but adding a new class for each hand type is unnecessary. The only difference between them is the rate at which they move, so add that as a property or state and the element (or maybe just the style) that they use and the hand is fully defined.

The CSS precision

There is the problem of CSS precision but that is easily fixed by getting the time at the start of the day and subtracting that from the current ms and does not need the extra rotation counter (not sure how long it needs to run for the second hand to start playing up again, but you can just reset that once a day if you plan on letting it run 24/7).

Rewrite

You had about 114 lines of javascript which can easily be reduced to 20 and behave exactly the same. There is no point in adding all the const for time as they are truly constants and you will never be hunting around to change them (The irony of constants is that you define them so it's easy to change them, not because they never change). I personally would have reduced the rate values eg 360 / 3600000 = 0.0001 but that is a little obscure so left them as is.

;(() => {
    const DAY_MS = 24 * 60 * 60 * 1000;
    const hand = (style, rate) => ms => style.transform = `rotate(${(ms * rate | 0) }deg)`;
    setInterval(() => requestAnimationFrame(display), 1000);
    const offset = new Date().getTimezoneOffset() * 60000;
    const start = (Date.now() / DAY_MS | 0) * DAY_MS;
    const $ = query => document.querySelector(query).style;
    const hands = [
        hand($('.second-hand'), 36 / 6000),
        hand($('.minute-hand'), 360 / 3600000),
        hand($('.hour-hand'), 360 / (12 * 3600000)),
    ];
    function display() {
        const ms = (Date.now() - offset) - start;
        hands.forEach(hand => hand(ms));
    }
})();

(*) Yes I know but some people take it personally.

\$\endgroup\$
9
  • \$\begingroup\$ While I agree that not every number deserves a named constant, one should also not fall into the trap of magical numbers. For example in your code it's hard for me to tell whether 36 is actually supposed to be 360 (like in other lines) because I'm not really sure what's the meaning of dividing it by 6000. Using named constants helps us express that meaning in code. \$\endgroup\$ Commented Nov 15, 2017 at 15:00
  • \$\begingroup\$ It seems to me that this code is sneaking to the realms of code golf, where the readability is sacrificed for brevity. For example your use of binary OR operator. I understand binary operations, but it takes me a while to wrap my head around what that code is trying to achieve. Why not simply express your actual intentions by using Math.floor(). \$\endgroup\$ Commented Nov 15, 2017 at 15:08
  • \$\begingroup\$ @ReneSaarsoo I will stop my lazy use of bitwise operators val|~~~-(0**0) yes much better. Is ** bad as well or should we use Math.pow() Don't you mean Math.trunc() as Math.floor() does not replace |0 . \$\endgroup\$
    – Blindman67
    Commented Nov 15, 2017 at 17:11
  • \$\begingroup\$ Indeed, Math.trunc() would be the correct equivalent of |0, but this code doesn't need to deal with negative numbers, so Math.floor() would work equally well. It's perfectly fine to use ** operator for exponentiation, as that's its intended usage. While using bitwise OR for truncating a number to integer is more of a side-effect of bitwise operations in JavaScript. If you really have a task where you want to do some bit-shifting, then bitwise operations are perfectly fine. \$\endgroup\$ Commented Nov 16, 2017 at 8:29
  • \$\begingroup\$ @Rene All bitwise operators truncate, it is not a "side-effect", Only >> >>> << "shift bits" though rotate, multiply, or divide are acceptable (eg 12.3 << 4 is equivalent to Math.trunc(12.3) * 16) Bitwise operations are essential in any language, shying away from them because there is lack of understanding is not acceptable and only compounds the ignorance. You had to work it out and now you have a broader knowledge of the language, and next time you have a line stretching to far to the right you may opt for a | 0 over the Math.floor(). \$\endgroup\$
    – Blindman67
    Commented Nov 16, 2017 at 11:35

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.