Desktop Notifications for Queue Items - Updated Code

Question

A while ago I took one of @Simon's scripts and updated it to make it more geared towards normal users (no offense mods) and this Question was created. But the link to the Github is no longer correct and the code has changed quite a bit to get it to work on Firefox and Chrome (and Opera and IceDragon and Safari).

I would like to know if my code is following UserScript and JavaScript standards and if there is anything that I can do to improve the efficiency of the code itself.

Here is the current Next version

Notification.requestPermission();
var KEY_NEXT = 'NextReload';
var DELAY = 120 * 1000; //120,000 milliseconds = 2 Minutes
var currentTime = Date.now ? Date.now() : new Date().getTime();
var lastTime = GM_getValue(KEY_NEXT, 0);
var nextTime = currentTime + DELAY;
GM_setValue(KEY_NEXT, nextTime);
var timeDiff = Math.abs(lastTime - currentTime);
setTimeout(function(){
 window.location.reload(); 
}, DELAY);
var notificationTitle = (document.title.split(' - ')[1] + ' Review Queue').replace(' Stack Exchange', '.SE');
// a way to detect that the script is being executed because of an automatic script reload, not by the user.
if (timeDiff <= DELAY * 2) {
 var reviewCount = 0;
 var reviewItems = document.getElementsByClassName('dashboard-num');
 for (var i = 0; i < reviewItems.length; i++){
 if (reviewItems[i].parentNode.className != 'dashboard-count dashboard-faded'){
 reviewCount += parseInt((reviewItems[i].getAttribute("title")).replace(',', ''), 10);
 console.log(reviewItems[i]);
 }
 }
 console.log(reviewCount);
 if (reviewCount > 0) {
 var details = {
 body: reviewCount + ' Review Items',
 icon: 'https://github.com/malachi26/ReviewQueueNotifier/raw/master/Resources/Icon2.jpg'
 } 
 var n = new Notification(notificationTitle, details);
 n.onclick = function(){
 window.focus();
 this.cancel();
 }
 setTimeout(n.close.bind(n), 100000); // Magic number is time to notification disappear 
 }
}

---

You can follow development or participate in development at Github as well.

Review Queue Notifier

• Jonah: Smallish point, but I'd replace that for loop with a forEach, and replace the embedded if statement with a returning guard clause for the case you want to ignore. This saves you a level of nesting and improves clarity. If you need old browser support and don't want to include the forEach polyfill, that's a decent recent to keep it as is.

Answer 1

Haven't actually tested the code, just writing JavaScript from the top of my head. Here's some notable changes:

• Using what I call "return early". That way, it avoids nested ifs and your code looks flat.

• Renamed a few variables so that they have more meaning, especially DELAY which is very generic and because you have 2 timeouts. Long names don't matter, readability should always be considered.

• Instead of using Math.abs, why not reverse the equation? Current time is always larger than time in the past. You'll always get a positive value.

• Replaced your loop with reduce because it was designed for that, an aggregator of values.

• You can always use the unary + as a shorthand for converting strings into numbers. In most cases, it acts similar to parseFloat. If the string is potentially not a number, guard yourself with isNan.

• Replaced your currentTime to just Date.now(). By now, browsers should at least be ES5-compliant. Anything less... you still rolling with IE6 or sumthin'?

• Notification title moved down nearer to notification and after the two early-return if. That way, you don't actually generate the title if the script bails out on either of the two returns.

• Inlined your notification details object. Assigning to a variable just took up space, only to be used once.

And the end result:

Notification.requestPermission();
var KEY_NEXT = 'NextReload';
var NOTIFICATION_AUTODISMISS_TIMEOUT = 100000;
var RELOAD_DELAY = 120 * 1000; //120,000 milliseconds = 2 Minutes
var currentTime = Date.now();
var lastTime = GM_getValue(KEY_NEXT, 0);
var timeDiff = currentTime - lastTime;
var nextTime = currentTime + RELOAD_DELAY;
GM_setValue(KEY_NEXT, nextTime);
setTimeout(window.location.reload, RELOAD_DELAY);
if (timeDiff > RELOAD_DELAY * 2) return;
var reviewItems = document.getElementsByClassName('dashboard-num');
var reviewItemsArray = Array.prototype.slice.call(reviewItems);
var reviewCount = reviewItemsArray.reduce(function(count, reviewItem){
 return +reviewItem.getAttribute('title').replace(',', '') + count;
}, 0);
if (reviewCount <= 0) return;
var notificationTitle = (document.title.split(' - ')[1] + ' Review Queue').replace(' Stack Exchange', '.SE');
var n = new Notification(notificationTitle, {
 body: reviewCount + ' Review Items',
 icon: 'https://github.com/malachi26/ReviewQueueNotifier/raw/master/Resources/Icon2.jpg'
});
n.onclick = function(){
 window.focus();
 this.cancel();
};
setTimeout(n.close.bind(n), NOTIFICATION_AUTODISMISS_TIMEOUT);

• janos: Nice! How do you feel about including the unit of time in the constants, for example RELOAD_MILLIS ?
• Joseph: @janos Sounds fine, although it's usually implied since JS usually works with milliseconds most of the time (setTimeout, setInterval, Date.now() all work with milliseconds).
• Malachi: Thank you for taking the time to review this code. I still consider myself a novice when it comes to Javascript so there were a couple of things I had to look up here, but a very enjoyable read, and a lot of things that make total sense, things that I should have seen and done, but didn't.
• Malachi: your code doesn't look like it will disregard the queues that are shadowed out, meaning that you cannot access those queues at this time. I can add an if statement inside that function though I think.
• Joseph: @Malachi I forgot reviewItems wasn't an array. Updated, converting it into an array with slice.