current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 7 down vote favorite

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" : "$1$3$4"); 
            }
            document.title = title + frags.slice(1).join(" ");
        } else { }

    });

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

        if(title.indexOf("[")> -1 && title.match(/\[/g).length == 2){
            var arr = title.split(/\s(?=\w)/), rtn =['[',,' • ',,']'];
            arr[0].split(" ").forEach(function(e){
                rtn[e.indexOf("+") > -1 ? 1 : 3] = e.replace(/\[|\]/g,"");
            });
            title = rtn.join("") + " " + arr.slice(1).join(" ");
        }
        return title;
    }
}
var el = document.createElement('script');
el.type = 'text/javascript';
el.text = '(' + userscript + ')(jQuery);';
document.head.appendChild(el);

What this does is when inbox item alone comes up, it shows as:

[{number of inbox item here}]

and same for rep change:

[+{rep change here}]

When both come, they will be displayed in the following style:

[+{rep change here} • {inbox items here}]

Also once the notifications are seen, it will be subsequently removed as well. I'm using MutationsObservers, which is warped by a plugin called jQuery.observe which looks to settle cross-browser issues.

How can this be improved?

share|improve this question

1 Answer 1

up vote 6 down vote accepted

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);
}
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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