18
\$\begingroup\$

For those who go to The 2nd Monitor chatroom, you already know how bad I am at remembering everything. And, sometimes, the new visitors will wonder what something means.

For that, I've developed a very simple chat translator.

;(function(window, undefined){
    'use strict';

    var memes = {
        'zombie': {
            'title': 'Unanswered question (or with answers without upvotes)',
            'href': 'http://meta.codereview.stackexchange.com/a/1511/',
            'find': new RegExp('(zombie)','gi')
        },
        'Jamalized': {
            'title': 'Being Jamalized means that Jamal edited your question/answer',
            'href': 'http://meta.codereview.stackexchange.com/a/1675/',
            'find': new RegExp('(jamalized)','gi')
        },
        'TS': {
            'title': 'Theoretical Star (star it and say "RSA")',
            'href': 'http://meta.codereview.stackexchange.com/a/1526/',
            'find': new RegExp('(TS)','g')
        },
        'RSA': {
            'title': 'Real Star Applied (you say it after staring a message with "TS")',
            'href': 'http://meta.codereview.stackexchange.com/a/1526/',
            'find': new RegExp('(RSA)','g')
        },
        'Thanks, Santa!': {
            'title': 'When someone upvotes a post, and you don\'t know who, just say this',
            'href': 'http://meta.codereview.stackexchange.com/a/1526/'
        },
        'IWNPFETTOLAI': {
            'title': 'I will not provide further explanation than this overly long acronym itself',
            'href': 'http://meta.codereview.stackexchange.com/a/1673/'
        },
        'Monking': {
            'title': 'A greeting to the Monkey doing his monkey-business',
            'href': 'http://meta.codereview.stackexchange.com/a/1678/',
            'find': new RegExp('(monk(?:ing|ernoon|evening|night))','gi')
        },
        'TTQW': {
            'title': 'Time To Quit Work',
            'href': 'http://meta.codereview.stackexchange.com/a/1643/'
        },
        'TTGH': {
            'title': 'Time To Go Home',
            'href': 'http://meta.codereview.stackexchange.com/a/1643/'
        },
        'TTGTB': {
            'title': 'Time To Go To Bed',
            'href': 'http://meta.codereview.stackexchange.com/a/1643/'
        },
        'STM': {
            'title': 'Smoking The Documentation',
            'href': 'http://meta.codereview.stackexchange.com/a/1953/'
        },
        'overengineering': {
            'title': 'A case of when something has become overly-engineered',
            'href': 'http://meta.codereview.stackexchange.com/a/2520/'
        },
        'HOLY CARP': {
            'title': 'Probably HOLY CRAP misspelled',
            'href': 'http://meta.codereview.stackexchange.com/a/4928/'
        },
        'Malachi\'d': {
            'title': 'Something amusing or entertaining that hasn\'t been starred yet',
            'href': 'http://meta.codereview.stackexchange.com/a/1667/'
        },
        'JDQ': {
            'title': 'JavaDeveloper Question',
            'href': 'http://meta.codereview.stackexchange.com/a/2053/'
        }
    },
    translate = function(){
        var messages = document.querySelectorAll('#chat .message:not([data-checked="1"])'),
            fragment = document.createDocumentFragment(),
            tmp_content = document.createElement('div');

        if( messages && messages.length )
        {
            for(var i = 0, l = messages.length; i < l; i++)
            {
                var message = messages[i];

                message.setAttribute('data-checked', '1');

                var content = message.querySelector('.content');

                if( content && !content.firstChild.className || content.firstChild.className.indexOf('onebox') === -1 )
                {
                    tmp_content.innerHTML = content.innerHTML;
                    for(var meme in memes)
                    {
                        var html = '';
                        for(var j = 0, m = tmp_content.childNodes.length; j < m; j++)
                        {
                            //text nodes only
                            if( tmp_content.childNodes[j].nodeType === 3 )
                            {
                                if( memes[meme].find )
                                {
                                    html += tmp_content
                                        .childNodes[j]
                                        .nodeValue
                                        .replace(memes[meme].find, '<a href="' + 
                                            ( memes[meme].href || '#' ) +
                                            '" target="_blank" title="' +
                                            ( memes[meme].title || '' ) +
                                            '" style="color:inherit;border-bottom:1px dashed #000">$1</a>'
                                        );
                                }
                                else
                                {
                                    html += tmp_content
                                        .childNodes[j]
                                        .nodeValue
                                        .replace(meme, '<a href="' + 
                                            ( memes[meme].href || '#' ) +
                                            '" target="_blank" title="' +
                                            ( memes[meme].title || '' ) +
                                            '" style="color:inherit;border-bottom:1px dashed #000">'+
                                            meme+
                                            '</a>'
                                        );
                                }
                            }
                            else if( tmp_content.childNodes[j].nodeType !== 8 )
                            {
                                html += tmp_content.childNodes[j].outerHTML;
                            }
                        }
                        tmp_content.innerHTML = html;
                    }
                    content.innerHTML = tmp_content.innerHTML;
                }
            }
        }
    }

    translate();

    setInterval(translate, 5000);

})(Function('return this')());

The idea is that the chat will be augmented, in a certain way, to add meanings to certain words or sentences.
Those will have a link and a descriptive title.

To run it, simply fire the console. Every 5 seconds, the chat will be scraped in search of new messages.

In terms of clarity and content, what can I change or improve?

\$\endgroup\$
  • 6
    \$\begingroup\$ 'Probably HOLY CRAP misppelled': should be misspelt \$\endgroup\$ – Quill Jul 13 '15 at 0:40
  • 3
    \$\begingroup\$ Holy Carp is in the same family as Holy Mackerel and Holy Halibut \$\endgroup\$ – Malachi Jul 13 '15 at 14:32
  • 2
    \$\begingroup\$ @EBrown Not really. Any edit is a Jamalization, as far as I know \$\endgroup\$ – Ismael Miguel Jul 13 '15 at 15:27
  • 1
    \$\begingroup\$ @IsmaelMiguel misspelled vs misspellt is a US vs UK English distinction. ( dictionary.cambridge.org/dictionary/british/misspell ). \$\endgroup\$ – TessellatingHeckler Jul 14 '15 at 15:22
  • 2
    \$\begingroup\$ There's no difference in meaning; misspelled is the American English dialect spelling, misspelt is the British English dialect spelling. I don't know why people are upvoting the correction - @Quill appears to be Australian which has a more British English background, but CodeReview/StackExchange are American sites, and most of the internet tends to use American English spellings more often. (Ugh, even as a native Brit, I'm getting the British spelling wrong. Removed the extra l). \$\endgroup\$ – TessellatingHeckler Jul 14 '15 at 17:38
6
\$\begingroup\$

Be careful when using setInterval:

setInterval(translate, 5000);

If for some reason something started to hang up, maybe your browser or your client's connection to the server, a thread will stall, and it will cause other threads to stack up because one is created every 5 seconds.

It would be safer to move this line to the end of your translate function:

setTimeout(translate, 5000);

This is safer because this ensures that the translate function will only be called again after the first one has been completed.


You have a few magic numbers in your code:

tmp_content.childNodes[j].nodeType !== 8

and

tmp_content.childNodes[j].nodeType === 3

You should create "constants" for these places so they are more clear. A constant would look something like this:

var CONSTANT_NAME = constant_value;

And, although you don't say it, if you happen to use ES6 in the future for this code, you could use the const keyword, rather than the var keyword:

const CONSTANT_NAME = constant_value;

At one point in your code, when you are doing some HTML substitution, you do this:

.replace(meme, '<a href="' + 
    ( memes[meme].href || '#' ) +
    '" target="_blank" title="' +
    ( memes[meme].title || '' ) +
    '" style="color:inherit;border-bottom:1px dashed #000">'+
    meme+
    '</a>'
 );

Why do you bother with these parts:

( memes[meme].href || '#' )

and

( memes[meme].title || '' )

All memes have a both a title and a href, so you do you check of it's existence? This seems unnecessary and you should just remove them from this chunk (and the next one).


These two chunks are very similar:

html += tmp_content
    .childNodes[j]
    .nodeValue
    .replace(meme, '<a href="' + 
        memes[meme].href+
        '" target="_blank" title="' +
        memes[meme].title +
        '" style="color:inherit;border-bottom:1px dashed #000">'+
        meme+
        '</a>'
     );

and

html += tmp_content
    .childNodes[j]
    .nodeValue
    .replace(memes[meme].find, '<a href="' + 
        memes[meme].href +
        '" target="_blank" title="' +
        memes[meme].title+
        '" style="color:inherit;border-bottom:1px dashed #000">$1</a>'
    );

Note: in these chunks, I included the change from my last recommendation.

The only differences are:

  • The first argument of .replace.

  • The innerHTML of the a tag.

You should build the string that you are going to be .replaceing before you enter these sections, so you can then make those minor changes. And, you should also store the string that is being .replaced since that is also the same for both conditional cases.

Here is what I mean:

var htmlStr = '<a href="' + 
            memes[meme].href +
            '" target="_blank" title="' +
            memes[meme].title+
            '" style="color:inherit;border-bottom:1px dashed #000">';
        ); // replacing the thing being replaced
var nodeValue = tmp_content.childNodes[j].nodeValue; // being replaced

if( memes[meme].find ) {
    html += nodeValue
        .replace(memes[meme].find, htmlStr + '$1</a>');

} else {
    html += nodeValue
        .replace(meme, htmlStr + meme + '</a>');
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Your last point is REALLY good. I tried to find a way and failed. So, I just did that way. But a better way would be to replace $1 with meme if there isn't a find. Or just set a replace property in the object. \$\endgroup\$ – Ismael Miguel Jul 14 '15 at 16:33
  • \$\begingroup\$ I've decided to mark this one as the accepted answer due to the fact that this answer touches more meaningful points, without breaking the code. \$\endgroup\$ – Ismael Miguel Aug 3 '15 at 17:16
13
\$\begingroup\$

And here I go:

  • Take advantage of the fact that SE sites use jQuery. This will trim down your DOM manipulation code to a high degree.

  • We usually have a rule in our team that the most you should nest for conditions is 2. If you have 3 or more nested, then it's time you use a function.

  • One problem I find problematic about nested if-else and other conditional blocks is that it's hard to trace what they changed. Try writing ala Functional Programming, where you call a function, pass in a value, and get a new value. Combine it with ternaries or hash maps and it ain't so bad.

  • I suggest against "one var for all vars" as it becomes hard to determine when the vars end and the real code starts. For instance, I tried cleaning your code, but the indent is off at first glance, then discovered the "one var".

  • Use "early return" if you happen to have if blocks with no else and nothing after it.

    function foo(){
      if(bar){
        // code at second level
      }
    }
    
    // is the same as
    
    function foo(){
      if(!bar) return;
      // code at first level
    }
    

Now my jQuery is a bit rusty, but I think your logic can be simplified to the following. It may not be complete, but you get the idea.

setInterval(function translate() {

  // Look for a child element that is not .onebox from .content that's under a non-checked ancestor .message in #chat
  // Phew! That's a long one!
  $('#chat .message:not([data-checked="1"]) .content > :not(.onebox)').each(function(index, content){

    // Mark the ancestor .message as checked.
    // We use attr() instead of .data() because we're using a selector to look, and data() doesn't alter attributes
    $(content).closest('.message').attr('data-checked', 1);

    var contentChildren = $(content).children();

    // Looking for text nodes with jQuery filter
    contentChildren.filter(function(index, element){

      return element.nodeType === 3;

    }).each(function(index, element){

      var html = $(element).text();

      $.each(memes, function(key, meme){
        html.replace(meme.find || meme, '<a href="' + meme.href + '" title="' + meme.title + '">' + meme + '</a>'); 
      });

      element.replaceWith(html);
    });
  });

}, 5000);
\$\endgroup\$
  • 3
    \$\begingroup\$ I really like your code. It's clean, easy to understand, really cleaver. But, it is missing an important point. The document fragment I use isn't an accident. It's HTML is refreshed every new meme. This avoids, for example, matching the RSA inside the title of the TS. Also, you forgot the style and the target on your HTML. Also, instead of $(content).closest('.message'), you could use $(content).parents('.message'), which I think that is faster. Other than that, I think that .content > :not(.onebox) could be rewritten as .content:not(:has(.onebox)) (continue) \$\endgroup\$ – Ismael Miguel Jul 13 '15 at 8:19
  • \$\begingroup\$ The only thing that comes to my mind, besides all that, is that you have a name in that function. The name there makes it a global function. Which means it can be accessed from the window object. (If I'm wrong, please correct me.) Also, you are creating the variable contentChildren. You could just chain the methods and they all would make sense. And still be readable. Other than that, it was a really great re-write. Sadly, I can't upvote for now since it has that problem described above. Other than that, there's no other reason to do not upvote. \$\endgroup\$ – Ismael Miguel Jul 13 '15 at 8:25
  • \$\begingroup\$ @IsmaelMiguel re: the name of the function, purely an accident (dem copy-paste :P). As for the rest, yes, it can be further simplified. I did leave out some of the logic as I was lost in trying to understand the entire thing. :) \$\endgroup\$ – Joseph Jul 13 '15 at 20:22
  • \$\begingroup\$ At least I have your input. But I don't mind about the logic. And I'm not asking you to re-write the entire thing for me. I'm warning you that there's a bug that can be fixed by copy-paste of parts of the code. Anyway, I will use your code, but I'm not sure how to handle the question. I'll be on 2ns Monitor. Ping me there later, if you wish \$\endgroup\$ – Ismael Miguel Jul 13 '15 at 20:47
3
\$\begingroup\$

You're matching the regexes in a loop, giving it \$O(n m)\$ performance instead of \$O(m)\$ where \$n\$ is the number of regexes and \$m\$ is the length of the text being matched.

Any halfway decent regex engine can match an arbitrary number of regexes simultaneously and then tell you which one matched (a trivial extension to the way a|b is matched; I've done this in C); I am not familiar enough with JavaScript to know if it has one.

Possibly since these regexes are fairly simple, you could capture a fixed piece of text and then use that in a dict lookup?

\$\endgroup\$
  • 1
    \$\begingroup\$ I'm sorry, but I'm having some dificulties to get your point. \$\endgroup\$ – Ismael Miguel Jul 13 '15 at 23:10
  • \$\begingroup\$ @IsmaelMiguel Which part exactly? Big-O notation in general, how good regex implementations work, or the possible workaround? \$\endgroup\$ – o11c Jul 13 '15 at 23:17
  • \$\begingroup\$ After almost 3 hours I finally got it! You're telling me to combine the matches I want the regexes to match case-insensitivelly. An object (what you call a dict on your answer) has case-sensitive values. That doesn't cope that well. \$\endgroup\$ – Ismael Miguel Jul 14 '15 at 1:14
  • \$\begingroup\$ @IsmaelMiguel then call .tolower() or whatever it's called in your language. \$\endgroup\$ – o11c Jul 14 '15 at 1:15
  • 1
    \$\begingroup\$ @IsmaelMiguel javascript supports named captures (but not natively). I think he is implying you take the concept of the answer and translate it (or have someone who is fluent in python translate it for you). I think the performance gain would be dubious though given that ti isn't supported natively and the overhead of another library + the method in which the library works \$\endgroup\$ – Dan Pantry Sep 1 '15 at 9:29
3
\$\begingroup\$

in your for loop I noticed that you were creating an m variable to hold the length of the childNodes and that isn't really necessary

 for(var j = 0, m = tmp_content.childNodes.length; j < m; j++)
 {

you can just do this instead

for(var j = 0; j < tmp_content.childNodes.length; j++)

if you are trying to increase the speed of the loop, and not access the length every loop, then you should do it like this

var m = tmp_content.childNodes.length;
for (var j = 0; j < m; j++)

it's a little cleaner, but it will be quicker than what I originally did, maybe just as fast as what you were doing. the variable does have a slightly different scope though.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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