Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

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:

carrots!

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!

share|improve this question
    
Please keep white spacing consistent... – Chris Cirefice Mar 16 at 13:46
up vote 11 down vote accepted

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.

share|improve this answer
4  
Right on the first point, the "ungolfed" version is missing the ;. Instead of your ( /-(\d+)/.exec(destinationMessage.id) || [])[1], one can use destinationMessage.id.replace(/^.*-(\d+).*$/, '$1'), which is more bullet-proof than your short .split('-'). Also, there's nothing wrong with e.id. Why waste time using the DOM when the DOM already wasted time for that e.id? Every single instance of Element has that attribute. It's a global attribute on DOM. – Ismael Miguel Mar 16 at 11:50
    
not using getAttribute is fine in this case but it often dissolves into bad practice with people using foo[computed_property] which is insecure and bad easily break. – Downgoat Jul 9 at 18:51
    
That's quite a late answer. Yes, I see where you are coming from, but, your practice also leads to things like e.getAttribute("onclick") and other weird stuffs. – Ismael Miguel Jul 9 at 22:23

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.

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.