HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMinor

Stack Overflow tab notifier user-script

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
scriptstackoverflowusertabnotifier

Problem

I searched for a notification script that shows inbox and rep change items on the browser tab in stackapps.com but either they didn't work or were outdated.

So, I've done this raw script myself and want to make it fool proof and robust:
This is my first ever user script. So want to make it better. (Even if it wasn't the first, I would have wanted to make it better)

```
// ==UserScript==
// @name Tab Notifier
// @namespace http://2200ce.wordpress.com/
// @version 0.1
// @description A notification system that displays inbox and rep change notification on the tab
// @author Amit Joki - http://stackoverflow.com/users/3001736/amit-joki
// @include http://stackoverflow.com/*
// @grant none
// @require https://cdn.rawgit.com/kapetan/jquery-observe/master/jquery-observe.js
// @match ://.stackexchange.com/*
// @match ://.stackoverflow.com/*
// @match ://.superuser.com/*
// @match ://.serverfault.com/*
// @match ://.askubuntu.com/*
// @match ://.stackapps.com/*
// @match ://.mathoverflow.net/*
// ==/UserScript==

var userscript = function($) {
$('.icon-inbox .unread-count, .icon-achievements .unread-count').observe({
attributes: true,
attributeFilter: ['style']
}, function() {
var title = document.title;
var isInbox = $(this).parent().is(".icon-inbox") ? true : false;
if (!$(this).attr('style')) {
document.title = updater(title, $(this), isInbox);
} else if ($(this).attr("style") == "display: none;") {
debugger;
var frags = title.replace(/• /,"•").split(/\s(?=\w)/);
var rgx = (isInbox ? /\[\d+\]/ : /\[\+\d+\]/);
if(frags[0].indexOf("•") == -1){
title = frags[0].replace(rgx, "") + frags.slice(1).join(" ");
} else {
rgx = /^(\[)(\+\d+) •(\d+)(\])/;
title = frags[0].replace(rgx, isInbox ? "$1$2$$4" :

Solution

Instead of this:

var isInbox  = $(this).parent().is(".icon-inbox") ? true : false;


Since .is(...) is boolean, you can use its returned value directly without the ternary operator:

var isInbox  = $(this).parent().is(".icon-inbox");


This else is pointless, just delete it:

} else { }


The indentation is off in the else block, and the formatting doesn't follow common conventions:

else if(rgx.test(title)){
        title = title.replace(rgx, "[" + txt + "] ");
    }
        else {title = "[" + txt + "] " + title;}


It would be better to write this code like this:

else if(rgx.test(title)){
        title = title.replace(rgx, "[" + txt + "] ");
    } else {
        title = "[" + txt + "] " + title;
    }


In this code:

var rgx = (isInbox ? /\[\d+\]/ : /\[\+\d+\]/);
if (title.indexOf("•") > -1) {
    rgx = isInbox ? /^(\• )\d+/ : /^(\[)+\d+/;
    title = title.replace(rgx, "$1" + txt);
}
// ...


It's misleading that rgx is first assigned to something,
then it's reassigned to something else inside the if branch.
This makes the reader look suspiciously at the code after the if statement to see if rgx is used again for something.

Looking at the code, rgx is not used again.
Which means,
you're just reusing the variable inside the if branch.
But it's not a good practice to reuse a variable for more than one purpose.
It would be better to create a new variable inside the if branch,
for example:

var rgx = (isInbox ? /\[\d+\]/ : /\[\+\d+\]/);
if (title.indexOf("•") > -1) {
    var rgx2 = isInbox ? /^(\• )\d+/ : /^(\[)+\d+/;
    title = title.replace(rgx2, "$1" + txt);
}

Code Snippets

var isInbox  = $(this).parent().is(".icon-inbox") ? true : false;
var isInbox  = $(this).parent().is(".icon-inbox");
else if(rgx.test(title)){
        title = title.replace(rgx, "[" + txt + "] ");
    }
        else {title = "[" + txt + "] " + title;}
else if(rgx.test(title)){
        title = title.replace(rgx, "[" + txt + "] ");
    } else {
        title = "[" + txt + "] " + title;
    }
var rgx = (isInbox ? /\[\d+\]/ : /\[\+\d+\]/);
if (title.indexOf("•") > -1) {
    rgx = isInbox ? /^(\• )\d+/ : /^(\[)+\d+/;
    title = title.replace(rgx, "$1" + txt);
}
// ...

Context

StackExchange Code Review Q#73557, answer score: 7

Revisions (0)

No revisions yet.