4
\$\begingroup\$

I wrote from the ground up this small piece of code which transforms dates and times formatted in ISO 8601 format into a more human readable format, client-side. The main goal is that you can create multiple and fully customizable instances that differ from each other for any given node living inside the DOM tree. It updates the time live if specified. I know there are some libraries out there that does it already, but they are way too big and rely on external frameworks. I intend to make it available for all on Github when it is ready.

As a beginner, I feel like I might have over complicated some stuff and that my code seems like a mess (spaghetti). I've been given the advice to write the code first (make it work), and then refactor. However, I am having issues with the refactor part, as I don't know where to start. Hence, this is why I am seeking for some advice/help in producing more efficient and understandable code.

First, you have to create some HTML element(s) with a custom selector and a custom attribute which holds the data generated by your application using a supported ISO 8601 format:

<time class="relative-time" datetime="2016-05-03T02:41:44.741Z"></time>

Then, you call the function, passing an object to it which contains all the specifications. The function then operates by itself.

relativeTime({
  node: ".relative-time",
  data: "datetime",
  prefix: {
    past: "",
    future: ""
  },
  suffix: {
    past: "ago",
    future: "from now"
  },
  second: {
    singular: "one second",
    plural: "%d seconds"
  },
  allowTimesUpdate: true,
  minute: {
    singular: "one minute",
    plural: "%d minutes"
  },
  hour: {
    singular: "an hour",
    plural: "%d hours"
  },
  day: {
    singular: "a day",
    plural: "%d days"
  },
  week: {
    singular: "a week",
    plural: "%d weeks"
  },
  month: {
    singular: "a month",
    plural: "%d months"
  },
  year: {
    singular: "a year",
    plural: "%d years"
  }
});

The actual code:

(function (window, document, undefined) {

  'use strict';

  if (NodeList.prototype.forEach === undefined) {
    NodeList.prototype.forEach = function (cb) {
      [].forEach.call(this, cb);
    };
  }

  function multiplyArrayItems(arr) {
    var sum = 1;
    for (var i = 0; i < arr.length; i++) {
      sum = sum * arr[i];
    }
    return sum;
  }

  function updateRelativeTime(fn, rate) {
    window.setTimeout(function () {
      if (window.requestAnimationFrame) {
        window.requestAnimationFrame(fn);
      } else {
        fn();
      }
    }, rate);
  }

  var relativeTime = function (spec) {
    function init() {
      var nodes = document.querySelectorAll(spec.node);

      nodes.forEach(function (node) {
        (function setRelativeTime() {
          var now = new Date().getTime();
          var data = new Date(node.getAttribute(spec.data)).getTime();
          var distance = (now - data) / 1000;
          var prefix = distance > 0 ? spec.prefix && spec.prefix.past ? spec.prefix.past : '' : spec.prefix && spec.prefix.future ? spec.prefix.future : '';
          var suffix = distance > 0 ? spec.suffix && spec.suffix.past ? spec.suffix.past : '' : spec.suffix && spec.suffix.future ? spec.suffix.future : '';

          distance = Math.abs(distance);

          var times = [60, 60, 24, 7, 4.35, 12];
          var units = [spec.second, spec.minute, spec.hour, spec.day, spec.week, spec.month, spec.year];

          for (var i = 0; distance >= times[i] && i < times.length; i++) {
            distance /= times[i];
          }

          distance = Math.floor(distance);

          var unit = distance <= 1 ? units[i].singular : units[i].plural;
          var relativeTime = unit.includes('%d') ? unit.replace('%d', distance) : unit;
          var output = prefix + ' ' + relativeTime + ' ' + suffix;

          node.textContent = output.trim();

          // Best way to reduce performance hit, at the cost of accuracy.
          // Attach a custom update rate according to the node's time unit
          // instead of refreshing every X amount of milliseconds
          var updateRate = multiplyArrayItems(times.splice(0, i)) * 1000;

          // Update will only occur when the user browser's tab is active and
          // will only attach the custom update rate if its value is
          // less than or equal to an hour
          if (spec.allowTimesUpdate === true && updateRate <= 3600000) {
            // Check if the node has not been removed from the DOM
            if (node.parentNode) {
              updateRelativeTime(setRelativeTime, updateRate);
            }
          }
        }());
      });
    }

    if (document.readyState === 'interactive' || document.readyState === 'complete') {
      init();
    } else {
      document.addEventListener('DOMContentLoaded', init, false);
    }
  };

  // Export as global
  window.relativeTime = relativeTime;

}(window, document));
Improve this question
\$\endgroup\$
3
  • 1
    \$\begingroup\$ If your code is intended as a purpose utility you should not be setting or relying on the NodeList.prototype.forEach You can not know if the prototype has been implemented by another utility script or if that implementation works as is expected. You can not know from one run to the next if that prototype is the one you set. Dont add common named functionality to built in objects without rigorous checking for compatibility. \$\endgroup\$
    – Blindman67
    Nov 20 '16 at 18:57
  • \$\begingroup\$ Fair enough. Would you recommend using a simple for loop instead? Like this: link. @Blindman67 \$\endgroup\$
    – scorchy
    Nov 20 '16 at 19:47
  • \$\begingroup\$ Yes a for or while loop is just as easy to use, and now you know that will work, rather than hoping it will. \$\endgroup\$
    – Blindman67
    Nov 20 '16 at 20:02
3
\$\begingroup\$

This review does likely not cover everything

Function definition

You are using two forms of function definition. I recommend using the style function relativeTime(spec) { .. } for that function, instead of the current notation with var. You can still export it with window.relativeTime = relativeTime;.

Ternary operator

You are using a chain of ternary operators to determine the right prefix and suffix. This makes it unreadable. Instead, split it up with a few if-statements:

var prefix;
var suffix;
if (distance > 0) {
  prefix = spec.prefix && spec.prefix.future ? spec.prefix.future : '';
  suffix = spec.suffix && spec.suffix.future ? spec.suffix.future : '';
} else {
  prefix = spec.prefix && spec.prefix.past ? spec.prefix.past : '';
  suffix = spec.suffix && spec.suffix.past ? spec.suffix.past : '';
}

Nesting functions

Nesting functions can sometimes be useful. However, I think you are over-doing it in the following snippet:

  var relativeTime = function (spec) {
    function init() {
      var nodes = document.querySelectorAll(spec.node);

      nodes.forEach(function (node) {
        (function setRelativeTime() {

Why not create a function setRelativeTime(node) and do the following?

nodes.forEach(setRelativeTime);

This makes the code a lot more readable.

Local data, namespace and OO

You are currently dropping your function in the global namespace. I would suggest creating a local namespace for your small library, and putting relativeTime under that. This is mainly to prevent accidental collisions with other libraries, and accidental collisions with possible future functionality of javascript itself.

You are currently fixing the "spec" of a number of nodes by calling a function, and creating functions in that function to fix the context. You could instead create a proper class, and create instances. You would end up with a new RelativeTime(spec); call probably.

Additionally, it would be easier to figure out which parts are meant to be the public interface, and which functions are "private".

Naming of functions

Having a function updateRelativeTime, relativeTime and setRelativeTime is more than confusing. Find better names for these functions.

Strictly speaking, the sum in multiplyArrayItems is actually a product. sum is what you get when doing addition.

Improve this answer
\$\endgroup\$
3
  • \$\begingroup\$ Hey! Thank you a lot, your advice are highly appreciated. I guess naming thing isn't my strong suit. So, I have done pretty much all of what you recommended. Only one thing tho is that removing setRelativeTime outside the forEach made the node acting weird when it was updating (with the updateRelativeTime fn) so I had to bind it, not sure if what I did is wrong/bad idea. I have posted ‘updated version’ code here : jsfiddle.net/egrLnqgn. If you could check it out and tell me what you think. Also, I have already thought of creating a class, just didn't really know how I would build it. \$\endgroup\$
    – scorchy
    Nov 20 '16 at 0:55
  • 1
    \$\begingroup\$ Well, you expect a function reference to be passed to updateNodeTime, so if you want to pass an argument to that function you have to indeed use bind. If you would just do updateNodeTime(setNodeTime(node), updateRate);, you execute the function right then, and call updateNodeTime with undefined (the return value of setNodeTime) and updateRate. You could instead change the signature of updateNodeTime and pass node as an argument of that function, then call fn(node) in updateNodeTime. \$\endgroup\$
    – Sumurai8
    Nov 20 '16 at 9:21
  • 1
    \$\begingroup\$ Something I need to point out: jslint disagrees with me declaring prefix and suffix twice, which is reasonable. I have updated my answer to reflect this. I have also not mentioned variable hoisting (docs). You can consider moving all declarations to the top of their context so it matches better with what actually happens. This is especially true for for-loops, where the var i is not actually in the loop-context, but in the context above it. \$\endgroup\$
    – Sumurai8
    Nov 20 '16 at 9: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.