2
\$\begingroup\$

I wrote a UserScript that auto-sorts contacts by offline time using the MutationObserver API. I tried to make it as DRY as I could think, and I also tried to make my code as straight-forward as possible, commenting thoroughly. Please let me know any improvements I can make.

// ==UserScript==
// @name         Facebook Messenger Chat Sorter
// @namespace    http://tampermonkey.net/
// @version      0.1
// @description  Auto-sort contacts by offline time
// @author       Patrick Roberts
// @match        https://www.facebook.com/*
// @grant        none
// ==/UserScript==

(function() {
  'use strict';

  // mutation observer to locate chat sidebar
  var exists = new MutationObserver(function(mutations) {
    // if chat sidebar found in any mutations
    if (mutations.some(function(mutation) {
      // check if element with class is added
      if (mutation.type === 'childList') {
        // return if any added element is the chat sidebar
        return Array.from(mutation.addedNodes).some(initFbChatSidebar);
      // check if class is changed in existing element
      } else {
        // return if changed element is the chat sidebar
        return initFbChatSidebar(mutation.target);
      }
    })) {
      // then de-initialize this observer
      exists.disconnect();
      exists.takeRecords();
    }
  });

  // start locating sidebar
  exists.observe(document.body, { attributes: true, childList: true, subtree: true, attributeFilter: ['class'] });

  // returns if any element is the chat sidebar, and inits it
  function initFbChatSidebar(el) {
    if (el instanceof HTMLElement && el.classList.contains('fbChatSidebar')) {
      initChatSort(el.querySelector('ul'));

      return true;
    }

    return false;
  }

  // initializes auto-sorting
  function initChatSort(ul) {
    var li = ul.children;

    // observer to sort contacts
    var observer = new MutationObserver(sort);

    // start sorting whenever list is mutated
    observer.observe(ul, { childList: true, characterData: true, subtree: true });

    // also sort on initialization since list is already there
    sort();

    function sort() {
      // invoke native sorting algorithm
      Array.from(li).sort(function(la, lb) {
        // get offline times
        var a = getOfflineTime(la);
        var b = getOfflineTime(lb);

        // compare offline times
        return a - b;
      }).forEach(function(li) {
        // add list items to list in sorted order
        ul.appendChild(li);
      });

      // remove mutations from queue caused by sorting
      observer.takeRecords();
    }
  }

  // parse offline time from contact list item
  function getOfflineTime(li) {
    var str = li.querySelector('[class$="on"] [class$="-"]').textContent;
    var num, unit;

    num = +str.slice(0, -1);
    unit = str.slice(-1);

    // multiply time by 60 if hour
    if (unit === 'h') {
      num *= 60;
    // multiply time by 60 * 24 if day
    } else if (unit === 'd') {
      num *= 60 * 24;
    }

    return num;
  }
})();
\$\endgroup\$
2
\$\begingroup\$

Normally I would suggest pulling up the functions. Though they get hoisted by JS, it's nice from a readability point of view that you know they exist first before code tries to use them.


// if chat sidebar found in any mutations
if (mutations.some(function(mutation) {

This is a huuuge if statement. You might want to consider pulling out that callback function. It doesn't use mutations anyways.


if (mutation.type === 'childList') {
    // return if any added element is the chat sidebar
    return Array.from(mutation.addedNodes).some(initFbChatSidebar);
  // check if class is changed in existing element
  } else {
    // return if changed element is the chat sidebar
    return initFbChatSidebar(mutation.target);
  }

Can be simplified into:

var isChildListMutation = mutation.type === 'childList';
return isChildListMutation  ? Array.from(mutation.addedNodes).some(initFbChatSidebar);
                            : initFbChatSidebar(mutation.target);

// start locating sidebar
exists.observe(document.body, { attributes: true, childList: true, subtree: true, attributeFilter: ['class'] });

Consider breaking the config into newlines. As a single line, it isn't really readable.


  Array.from(li).sort(function(la, lb) {
    // get offline times
    var a = getOfflineTime(la);
    var b = getOfflineTime(lb);

    // compare offline times
    return a - b;
  }).forEach(function(li) {
    // add list items to list in sorted order
    ul.appendChild(li);
  });

Consider appending to a document fragment instead of directly to the DOM. Only after you append every li to the fragment, append the fragment to the ul.


var num, unit;

num = +str.slice(0, -1);
unit = str.slice(-1);

Consider putting var on the assignment operation instead. Save an extra line. Additionally, your num is not guarded against NaN.

\$\endgroup\$
  • \$\begingroup\$ I like almost all of your points except the one about the document fragment. Wouldn't that throw an error considering that they're already children of the ul? calling appendChild with the argument being an element that already has a parent throws an error iirc, unless the parent that appendChild is invoked on, is the parent of that child element already. In this specific case, it just moves it to the end like I'm already trying to do. \$\endgroup\$ – Patrick Roberts Jun 13 '16 at 2:50
  • \$\begingroup\$ @PatrickRoberts "calling appendChild with the argument being an element that already has a parent throws an error iirc" - not really. jsfiddle.net/6zxqzwbx \$\endgroup\$ – Joseph Jun 13 '16 at 3:05
  • \$\begingroup\$ Fair enough.. I wonder why I thought that in the first place. I remember doing something a few years ago that somehow caused appendChild to throw an error. Very nice review, thank you for the effort. \$\endgroup\$ – Patrick Roberts Jun 13 '16 at 3:17

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.