Stack Exchange Chat Caret Pathfinder

Question

In The Nineteenth Byte, we have an excessive amount of carets (carrots, if you will) that direct one's attention to another message. For example:

Amidst the confusion, I made a userscript that highlights in yellow the pointed-at message when the message in question is clicked.

(function(){
    var elements = {}, messages;

    function findCarrotMessage(){
        messages = Array.prototype.slice.call(document.querySelectorAll("[id^='message-']"));
        messages.forEach(function(e){
            if(!elements[e.id] && e.className==="message"){
                elements[e.id] = true;
                e.addEventListener("click", function(s){
                    // count the number of ^'s
                    var num = this.textContent.match(/\^+/g), self = this;
                    if(num){
                        var maxHash = -1;
                        num.forEach(function(k){
                            var destinationMessage = messages[Array.from(messages).indexOf(self)-k.length];
                            destinationMessage.style.background = "yellow";
                            setTimeout(function(e){e.style.background = "";}, 3500, destinationMessage);
                            maxHash = Math.max(destinationMessage.id.slice(destinationMessage.id.indexOf("-")+1), maxHash);
                            elements[self.id] = false;
                        });
                        window.location.hash = "#message-" + maxHash;
                    }
                    this.removeEventListener("click", arguments.callee, false);
                }, false);
            }
        });
    }

    setInterval(findCarrotMessage,50);
})();

How can I improve my code? Thanks!

Answer 1

There is still code-golf

Here you have some golfed code:

function(e){e.style.background = "";}

ungolf it:

function(e) {
    e.style.background = ""
}

Run away from arguments.callee

The 5th edition of ECMAScript (ES5) forbids use of arguments.callee() in strict mode. Avoid using arguments.callee() by either giving function expressions a name or use a function declaration where a function must call itself.

 - arguments.callee on MDN

---

here you have:

e.addEventListener("click", function(s){

instead do...

var MyFunction = function(s) {

and then...

e.addEventListener("click", MyFunction);

this allows you to then do:

this.removeEventListener("click", MyFunction, false);

Don't forget whitespace:

e.className==="message"

add whitespace around the ===. There a lot more places where spaces could be added

Conflicting things

In one part you're using:

 Array.prototype.slice.call

in another you are using:

Array.from

you should probably decide on one

Don't to a ton of things in one line

maxHash = Math.max(destinationMessage.id.slice(destinationMessage.id.indexOf("-")+1), maxHash)

that is waaaay to much in one line for a code reviewer like me ;) to be supporting. This:

destinationMessage.id.slice(destinationMessage.id.indexOf("-")+1)

looks like it can become a regex:

( /-(\d+)/.exec(destinationMessage.id) || [])[1]

or even better:

destinationMessage.id.split("-")[1]

use .getAttribute

rather than something like:

e.id

prefer:

e.getAttribute("id")

Remove all code-golf

var destinationMessage = messages[Array.from(messages).indexOf(self)-k.length]

you should split this into multiple variables. What if indexOf returns -1, you should handle that because otherwise your code will error and completely breakdown

use .bind rather than self = this

You're having to introduce self because the forEach's function creates a new scope. Instead use .bind and keep your old scope

num.forEach(function(k) {
    // code
}.bind(this))

Even better, use e.target to get the element that's been clicked. I highly recommending avoiding using this in lambdas to store data.

Answer 2

One line variable declaration

Please avoid declaring variables on the same line:

var elements = {}, messages;

It's also a great way to royally mess up scope issues if you do it on two lines but still with spaces.

var notAGlobal //, a comma might be here
    aGlobal;

---

Globals

messages doesn't need to be a global, you reassign it every time you call the function.

---

apply vs call

I always found that apply was more useful than call, in that you can specify parameters as an array (vs manually listing them). There's not much of a difference, but it's still good to know your options.

See this SO answer for more info

---

Indentation level

Anything that looks like this can be cleaned up:

function(){
   if(!whatever){
       //...
   }
}

You can reverse the condition and continue or return to reduce the level of indentation in some of your code:

if(!elements[e.id] && e.className==="message"){

---

Missing whitespace

You're missing whitespace in a few lines, like this one:

if(!elements[e.id] && e.className==="message"){

You should always have whitespace on both sides of a binary operator.

setTimeout(function(e){e.style.background = "";}

---

setTimeout

Instead of using setTimeout, you can bind the function to an eventListener.

Specifically, if you use the DOMNodeInserted event, you can call the message check every time a node is added (this will trigger on stars, but hey, if nothing happens, then nothing happens)

If you did, however, want to take the event triggering to a lower amount, you could consider using MutationObservers, but I always found them painful.

window.addEventListener('DOMNodeInserted', findCarrotMessage);

---

findCarrotMessage

This name is confusing and incorrect.

It doesn't find carrot messages, it iterates through the messages and replaces text.

You should consider breaking this function up into smaller functions and using a name that suits (and accurately describes the action) beyond a vague description.

---

Array.from(messages).indexOf(self)-k.length

Uh, the second parameter in Array.prototype.forEach is the index of the item. Use that instead of recasting the array.

However, if you had a genuine reason to recast the array, you shouldn't do it that way. The third parameter of Array.prototype.forEach is the array itself. (wow, it's almost like they thought about these things :p)

---

maxHash:

This would be better termed lastHash than maxHash.

You shouldn't have single lines this complex:

Math.max(destinationMessage.id.slice(destinationMessage.id.indexOf("-")+1), maxHash);

You should break this up into components before something gets that complex in a single line.